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

Lazy-install sourceMapSupport #6651

Merged

Conversation

@aminmarashi-binary
Copy link
Contributor

aminmarashi-binary commented Oct 31, 2017

Q                       A
Fixed Issues? Fixes #5890
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR
Any Dependency Changes?

This is an attempt to fix issue #5890 according to @loganfsmyth suggestion.

Todo:

  • Check if this actually fixes the performance problem
  • Make sure the fix is not a breaking change
  • Make sure the source maps are created correctly when requested
  • Add tests for modules with/without source maps
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Oct 31, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5635/

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Oct 31, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5558/

maps[filename] = result.map;
if (result.map) {
// Install sourceMapSupport on receiving the first map
if (!Object.keys(maps).length) {

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 31, 2017

Member

Can just be .length > 0 right?

This comment has been minimized.

Copy link
@aminmarashi-binary

aminmarashi-binary Oct 31, 2017

Author Contributor

No, actually what I meant was .length == 0 to run the installation only once, when nothing has been added to the maps yet.
I agree that the above syntax is not always easily readable, but it's the shortest check for object emptiness that I know of.

This comment has been minimized.

Copy link
@hzoo

hzoo Nov 1, 2017

Member

ah that's why I had that comment below I suppose 😛

if (result.map) {
// Install sourceMapSupport on receiving the first map
if (!Object.keys(maps).length) {
installSourceMapSupport();

This comment has been minimized.

Copy link
@hzoo

hzoo Oct 31, 2017

Member

We need to make this only run once right?

@hzoo hzoo requested a review from loganfsmyth Nov 1, 2017
@aminmarashi-binary aminmarashi-binary force-pushed the aminmarashi-binary:lazy-install-source-map branch from 494411f to 3368b72 Nov 2, 2017
@aminmarashi-binary

This comment has been minimized.

Copy link
Contributor Author

aminmarashi-binary commented Nov 2, 2017

@hzoo @loganfsmyth I updated the changes. Adding some tests now...

@aminmarashi-binary

This comment has been minimized.

Copy link
Contributor Author

aminmarashi-binary commented Nov 2, 2017

I added the tests as well.

@@ -69,4 +75,32 @@ describe("@babel/register", function() {
})
.to.throw(SyntaxError);
});

it("Does not install source map support if asked not to", () => {

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Nov 2, 2017

Member

capitalizing the first letter of all these it blocks is inconsistent with the rest of the tests in this file.

ast: false,
});

if (extendedOpts.sourceMaps) {

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Nov 2, 2017

Member

I think separating the installation from the insertion of result.map is somewhat confusing since you're relying on the inputs to Babel instead of the outputs now. What was the motivation for moving this, vs where it was in earlier implementations? I'd probably say

if (result.map) maps[filename] = result.map;
installSourceMapSupport();

and do

if (Object.keys(maps).length === 0) return;

at the top of installSourceMapSupport.

This comment has been minimized.

Copy link
@aminmarashi-binary

aminmarashi-binary Nov 3, 2017

Author Contributor

Yes, makes sense. Actually, I did that to run the install function before babel transform, which wasn't necessary.

Copy link
Member

loganfsmyth left a comment

Just a few small comments.

@@ -67,7 +70,7 @@ function compile(code, filename) {
// Do not process config files since has already been done with the OptionManager
// calls above and would introduce duplicates.
babelrc: false,
sourceMaps: "both",
sourceMaps: "sourceMaps" in opts ? opts.sourceMaps : "both",

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Nov 3, 2017

Member

Oh one last comment, sorry. It's technically fine to have the key but have it set to undefined, so I'd probably vote for opts.sourceMaps === undefined ? "both" : opts.sourceMaps.

This comment has been minimized.

Copy link
@aminmarashi-binary

aminmarashi-binary Nov 3, 2017

Author Contributor

This might cause some confusion, as it might be the user's intention to pass undefined as falsy, and this way we are not allowing that.

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Nov 3, 2017

Member

It's technically possible, but at the end of the day that's not what it means in our options processing. undefined means non-existant, so for instance if you have sourceMaps: true in your .babelrc, passing sourceMaps: false in the programmatic options would override true to false, whereas sourceMaps: undefined would have no effect.

This comment has been minimized.

Copy link
@aminmarashi-binary

aminmarashi-binary Nov 3, 2017

Author Contributor

Right, making the change.

@@ -77,7 +79,12 @@ function compile(code, filename) {
result.mtime = mtime(filename);
}

maps[filename] = result.map;
if (result.map) {
if (Object.keys(maps).length === 0) {

This comment has been minimized.

Copy link
@aminmarashi-binary

aminmarashi-binary Nov 3, 2017

Author Contributor

@loganfsmyth I added this change to run the install function only once.

This comment has been minimized.

Copy link
@frenzzy

frenzzy Nov 9, 2017

Is it not required to run sourceMapSupport.install() for each file?

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Nov 9, 2017

Member

Nope, it's installed once globally, so now we install it the first time we see that we have a sourcemap.

@loganfsmyth loganfsmyth merged commit 83cd3fb into babel:master Nov 9, 2017
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.95% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Nov 9, 2017

Thanks!

@aminmarashi-binary aminmarashi-binary deleted the aminmarashi-binary:lazy-install-source-map branch Dec 3, 2017
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.