Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not inline sourcemaps in jake - source-map-support can't handle it #17732

Merged
merged 2 commits into from
Aug 10, 2017

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Aug 10, 2017

Internally, source-map-support (which we enable during tests/debugging for sourcemapped stack traces, and recently started inspecting during tests when we stopped using a noop logger for them) uses a regexp to find the source mapping url - except with inline sources and inline source maps, we have a 25MB file it operates over. With a backtracking regexp, this sometimes triggers this v8 behavior (if the heap is in heavy use - like when we're deep inside our test suite in a single process). This was causing tests to fail on our internal CI server. Best I can tell, this doesn't happen on travis because the memory pressure is different (specifically, travis is already on node 8.3, while both myself and the CI server (the only machines which repo'd this issue) were on node 8.1.2 - best I can tell, 8.1.3 apparently reduced the memory usage of the profiler API by instantiating fewer regexes in the node profiler polyfill).

The fix is to just not use 15MB of inline sourcemaps. In the Jakefile, we were doing this to enable sourcemaps to work with runtests-browser - but we've had this working for awhile without using inline sources in our implementation in the Gulpfile. So now the Jakefile no longer builds tests with inline sourcemaps and shells out to gulp to build tests for the browser.

If you're using jake, you should not notice much of a difference in your workflow.

Jakefile.js Outdated
var cmd = 'browserify built/local/run.js -t ./scripts/browserify-optional -d -o built/local/bundle.js';
task("browserify", [], function() {
// Shell out to `gulp`, since we do the work to handle sourcemaps correctly w/o inline maps there
var cmd = 'gulp browserify';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add --silent to this command to avoid all the excess gulp output.

@weswigham weswigham merged commit a6a27ab into microsoft:master Aug 10, 2017
@weswigham weswigham deleted the put-maps-down-and-shell-out branch August 10, 2017 21:34
@mihailik
Copy link
Contributor

Does it blow up inside external code, or inside typescript?

If it's typescript, it would mean a bug — similar errors might show up in other use cases. For example, we're routinely inlining sources, not even sourcemaps in many of our apps. Hopefully typescript won't start to break on that?

@weswigham
Copy link
Member Author

weswigham commented Aug 11, 2017 via email

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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