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

CommonJS wrapper doesn't like being bundled #833

Closed
OliverJAsh opened this Issue Jul 16, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@OliverJAsh

OliverJAsh commented Jul 16, 2015

I have a third party script which is an optimized AMD file that contains a CommonJS wrapper:

// bar.js
define('bar', function(require) {
    var foo = require("./foo");
});
define('foo', function () {});

When I try to bundle this file into my own file:

// app.js
define(['bar'], function () {});

r.js doesn't notice that foo is already defined within the bar file, leading to:

>> Error: ENOENT, no such file or directory
>> '/Users/OliverJAsh/Development/frontend/foo.js'
>> In module tree:
>>     app

I'm guessing this is because r.js has to perform static analysis of the CommonJS wrapper. What do you recommend to work around this issue?

@jrburke jrburke added this to the 2.1.20 milestone Jul 17, 2015

@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Jul 17, 2015

Member

Sounds like it could be two different things:

  1. If the file is a result of some other concatenation process and it has a UMD wrapper around it, ideally the optimizer would not be diving inside the inner part of the UMD wrapper that had these define() calls if they are not publicly visible. If that sounds like the issue, getting a more complete look at the structure of the file would help. The optimizer does try to avoid looking inside UMD wrapped files, but as is the case with static analysis of JS, there can be multiple ways to construct the UMD wrapper and that particular signature may creep by the existing analysis.

  2. If there is no wrapper around the define() calls, and the examples are basically the contents minus the actual factory function definitions, I see that failing locally too, and I will want to fix that part. Looks like it will actually be a fix in requirejs to account for items in the define() queue.

The main bug in 2) is that it should not really matter what order the define() calls are in, but unfortunately looks like a bug got in where the order of dependencies is assumed to be least dependencies to more dependencies (so assuming define('foo') would come before define('bar') in this example).

As for workarounds, if it is just an issue of a build and the only issue is 2), rawText config could be used to seed the 'foo' value. It would likely mean a duplicate define('foo') in the output, but that is fine. However, if the real code has an actual factory function definition for foo, and it is not just a function(){} stub, then that will not be the path to take.

You could use the onBuildRead/onBuildWrite hooks to seed some stub define('foo') at the top of the file with onBuildRead just to get it the optimizer to see it first, then maybe remove that stub with onBuildWrite.

Or if you have control over how that bar.js file concatenates the modules, ordering them where leaf dependencies are first should avoid the bug.

However, if there is a UMD wrapper around the define() calls (issue 1), then it would be good to get a more fleshed out example of the file's structure.

Member

jrburke commented Jul 17, 2015

Sounds like it could be two different things:

  1. If the file is a result of some other concatenation process and it has a UMD wrapper around it, ideally the optimizer would not be diving inside the inner part of the UMD wrapper that had these define() calls if they are not publicly visible. If that sounds like the issue, getting a more complete look at the structure of the file would help. The optimizer does try to avoid looking inside UMD wrapped files, but as is the case with static analysis of JS, there can be multiple ways to construct the UMD wrapper and that particular signature may creep by the existing analysis.

  2. If there is no wrapper around the define() calls, and the examples are basically the contents minus the actual factory function definitions, I see that failing locally too, and I will want to fix that part. Looks like it will actually be a fix in requirejs to account for items in the define() queue.

The main bug in 2) is that it should not really matter what order the define() calls are in, but unfortunately looks like a bug got in where the order of dependencies is assumed to be least dependencies to more dependencies (so assuming define('foo') would come before define('bar') in this example).

As for workarounds, if it is just an issue of a build and the only issue is 2), rawText config could be used to seed the 'foo' value. It would likely mean a duplicate define('foo') in the output, but that is fine. However, if the real code has an actual factory function definition for foo, and it is not just a function(){} stub, then that will not be the path to take.

You could use the onBuildRead/onBuildWrite hooks to seed some stub define('foo') at the top of the file with onBuildRead just to get it the optimizer to see it first, then maybe remove that stub with onBuildWrite.

Or if you have control over how that bar.js file concatenates the modules, ordering them where leaf dependencies are first should avoid the bug.

However, if there is a UMD wrapper around the define() calls (issue 1), then it would be good to get a more fleshed out example of the file's structure.

@OliverJAsh

This comment has been minimized.

Show comment
Hide comment
@jrburke

This comment has been minimized.

Show comment
Hide comment
@jrburke

jrburke Jul 17, 2015

Member

Thanks for the example file. It was indeed the UMD detection not correctly identifying the UMD wrapper. I filed requirejs/requirejs#1389 to address the separate, unrelated issue of allowing out of order define() calls (the 2) issue above), just happened across that separate bug while looking into this one.

The fix for this issue will be in 2.1.20. I am not sure yet when I will publish it, but given that UMD wrapped files have been showing up more often, I am thinking on the order of a week or so. Do not want to go too long without this fix going out. I may even get it out this weekend, just want to scrub through some other open issues before locking down the release.

If you want a fix today, the r.js master snapshot contains the fix:

https://raw.githubusercontent.com/jrburke/r.js/master/dist/r.js

For this specific fix on top of 2.1.19 (where the above link will change as master changes):

https://raw.githubusercontent.com/jrburke/r.js/3407e2d45ff6dfd268b8e3e13f7c712180daf6ad/dist/r.js

Member

jrburke commented Jul 17, 2015

Thanks for the example file. It was indeed the UMD detection not correctly identifying the UMD wrapper. I filed requirejs/requirejs#1389 to address the separate, unrelated issue of allowing out of order define() calls (the 2) issue above), just happened across that separate bug while looking into this one.

The fix for this issue will be in 2.1.20. I am not sure yet when I will publish it, but given that UMD wrapped files have been showing up more often, I am thinking on the order of a week or so. Do not want to go too long without this fix going out. I may even get it out this weekend, just want to scrub through some other open issues before locking down the release.

If you want a fix today, the r.js master snapshot contains the fix:

https://raw.githubusercontent.com/jrburke/r.js/master/dist/r.js

For this specific fix on top of 2.1.19 (where the above link will change as master changes):

https://raw.githubusercontent.com/jrburke/r.js/3407e2d45ff6dfd268b8e3e13f7c712180daf6ad/dist/r.js

@OliverJAsh

This comment has been minimized.

Show comment
Hide comment
@OliverJAsh

OliverJAsh Jul 19, 2015

Thank you very much for the fast response! :-)

On Fri, 17 Jul 2015 at 18:17 James Burke notifications@github.com wrote:

Thanks for the example file. It was indeed the UMD detection not correctly
identifying the UMD wrapper. I filed requirejs/requirejs#1389
requirejs/requirejs#1389 to address the
separate, unrelated issue of allowing out of order define() calls (the 2)
issue above), just happened across that separate bug while looking into
this one.

The fix for this issue will be in 2.1.20. I am not sure yet when I will
publish it, but given that UMD wrapped files have been showing up more
often, I am thinking on the order of a week or so. Do not want to go too
long without this fix going out. I may even get it out this weekend, just
want to scrub through some other open issues before locking down the
release.

If you want a fix today, the r.js master snapshot contains the fix:

https://raw.githubusercontent.com/jrburke/r.js/master/dist/r.js

For this specific fix on top of 2.1.19 (where the above link will change
as master changes):

https://raw.githubusercontent.com/jrburke/r.js/3407e2d45ff6dfd268b8e3e13f7c712180daf6ad/dist/r.js


Reply to this email directly or view it on GitHub
#833 (comment).

OliverJAsh commented Jul 19, 2015

Thank you very much for the fast response! :-)

On Fri, 17 Jul 2015 at 18:17 James Burke notifications@github.com wrote:

Thanks for the example file. It was indeed the UMD detection not correctly
identifying the UMD wrapper. I filed requirejs/requirejs#1389
requirejs/requirejs#1389 to address the
separate, unrelated issue of allowing out of order define() calls (the 2)
issue above), just happened across that separate bug while looking into
this one.

The fix for this issue will be in 2.1.20. I am not sure yet when I will
publish it, but given that UMD wrapped files have been showing up more
often, I am thinking on the order of a week or so. Do not want to go too
long without this fix going out. I may even get it out this weekend, just
want to scrub through some other open issues before locking down the
release.

If you want a fix today, the r.js master snapshot contains the fix:

https://raw.githubusercontent.com/jrburke/r.js/master/dist/r.js

For this specific fix on top of 2.1.19 (where the above link will change
as master changes):

https://raw.githubusercontent.com/jrburke/r.js/3407e2d45ff6dfd268b8e3e13f7c712180daf6ad/dist/r.js


Reply to this email directly or view it on GitHub
#833 (comment).

@OliverJAsh OliverJAsh referenced this issue Jul 31, 2015

Merged

Use when’s ES6 Promise shim #9972

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment