Skip to content

use Harness.IO instead of sys in harness#4394

Merged
vladima merged 4 commits into
masterfrom
browserifyFix
Aug 21, 2015
Merged

use Harness.IO instead of sys in harness#4394
vladima merged 4 commits into
masterfrom
browserifyFix

Conversation

@vladima

@vladima vladima commented Aug 21, 2015

Copy link
Copy Markdown
Contributor

alternative fix for #4359.

It is ok for sys to be undefined since it is just an implementation detail of the default version of CompilerHost. Compiler and language service can already handle this. However test harness still uses sys all over the place, in this PR usages of sys in tests were replaced with Harness.IO. Also I've kept only fs in 'package.json' since this is the only module we use that is not shimmed by the 'webpack'.

@vladima

vladima commented Aug 21, 2015

Copy link
Copy Markdown
Contributor Author

ok, something is broken I need to re-run tests on Linux box since on Windows machine everything works fine

@jbrantly

Copy link
Copy Markdown

The browser stuff in package.json is so that webpack/browserify don't shim those modules. They're not actually needed at runtime in the browser and shimming them just adds to the filesize without actually doing anything.

@vladima

vladima commented Aug 21, 2015

Copy link
Copy Markdown
Contributor Author

I'm ok with putting back 'fs', 'os' and 'path' since these modules should not be used in tests. However currently test harness uses 'buffer' so it needs to be shimmed. It can be re-added to 'package.json' if there is a way to tell browserify cli to either ignore 'package.json' or include some module if even if it was skipped in "browser" section - after few minutes of experimenting I have not found it.

@vladima

vladima commented Aug 21, 2015

Copy link
Copy Markdown
Contributor Author

pinging @mhegazy

@jbrantly

Copy link
Copy Markdown

currently test harness uses 'buffer' so it needs to be shimmed

Gotcha. +1, I think this is much better than rolling back #4308

@mhegazy

mhegazy commented Aug 21, 2015

Copy link
Copy Markdown
Contributor

👍

vladima added a commit that referenced this pull request Aug 21, 2015
use Harness.IO instead of sys in harness
@vladima vladima merged commit ea1512e into master Aug 21, 2015
@vladima vladima deleted the browserifyFix branch August 21, 2015 21:25
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants