Fix resolving of local common js wrapped modules. #4529

zastavnitskiy · 2017-03-21T21:08:55Z

What kind of change does this PR introduce?
Bugfix for #4511

Did you add tests for your changes?
Not really, struggling to understand where and hot add them. Help appreciated.

Summary
Currenly, commonjs wrapped local modules are not resolved. This is a fix.

Does this PR introduce a breaking change?
No

sokra · 2017-03-21T21:26:37Z

lib/dependencies/AMDDefineDependency.js

@@ -57,7 +57,7 @@ AMDDefineDependency.Template = class AMDDefineDependencyTemplate {
 			],
 			lf: [
 				"var XXX;",
-				"!(XXX = #.call(exports, __webpack_require__, exports, module))"
+				"!(XXX = #.call(exports, __webpack_require__, exports, module)); XXX === undefined && (XXX = module.exports)"


I must stay a single expression. Not a statement.

define could be used this way:

a && define(...) && b

Also fix the lof case.

Passing exports and module from the surrounding module is wrong here too.

Maybe this could fix it:

var XXX, XXX_module; !(XXX_module = { id: YYY, exports: {}, loaded: false }, XXX = #.call(XXX_module.exports, __webpack_require__, XXX_module.exports, XXX_module), XXX_module.loaded = true, XXX === undefined && (XXX = XXX_module.exports) )

Hey, thanks for the review! Can you please explain a bit about

Passing exports and module from the surrounding module is wrong here too.

I don't really get why that's wrong.

sokra

Please also add tests

zastavnitskiy

@sokra Could you please take a look? I'm a bit unsure about module id — where do I take it from and how is it used later on?

sokra

Looks good.

We still need a test for this.

sokra · 2017-03-23T07:49:23Z

lib/dependencies/AMDDefineDependency.js

+				 XXX = (typeof XXXfactory === 'function' ?
+				 (XXXfactory.call(XXXmodule.exports, __webpack_require__, XXXmodule.exports, XXXmodule)) : XXXfactory),
+				 (XXXmodule.loaded = true),
+				 XXX === undefined && (XXX = XXXmodule.exports))`


Please don't embed newlines/tabs into the generated source. This blow up the generated code with webpack stuff. Best keep it as equal to the original source as possible. Just generate a single line with this AMD wrapper code, that hides it the best.

Hey, I added the id, removed new lines and added tests. Please, take a look once you will have time.

sokra · 2017-03-23T07:52:50Z

For id best use the user provided id.

define("nameOfModule", ...)

But this is not yet available in AMDDefineDependency. You need to pass it from AMDDefineParserPlugin into AMDDefineDependency. Just add a constructor argument.

webpack-bot · 2017-03-30T12:51:21Z

@zastavnitskiy Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

sokra · 2017-03-31T00:37:10Z

test/cases/resolving/commomjs-local-module/index.js

+});
+
+define("override-exports", function(require, exports, module) {
+  exports = "this one overrides exports reference";


This won't work.

Setting exports is not a valid way to export stuff.

It doesn't, but we should return empty object and not undefined as it was before.

sokra · 2017-03-31T00:38:27Z

lib/dependencies/AMDDefineDependency.js

-				"var XXX;",
-				"!(XXX = #.call(exports, __webpack_require__, exports, module))"
+				"var XXX,XXXmodule;",
+				"!(XXXmodule = { id: 'YYY', exports: {}, loaded: false }, XXX = #.call(XXXmodule.exports, __webpack_require__, XXXmodule.exports, XXXmodule),XXXmodule.loaded = true,XXX === undefined && (XXX = XXXmodule.exports))"


'YYY' -> YYY

sokra · 2017-03-31T00:38:53Z

lib/dependencies/AMDDefineDependency.js

@@ -112,6 +112,10 @@ AMDDefineDependency.Template = class AMDDefineDependencyTemplate {
 			definition = definition.replace(/XXX/g, localModuleVar.replace(/\$/g, "$$$$"));
 		}

+		if(dependency.namedModule) {
+			text = text.replace(/YYY/g, dependency.namedModule);


dependency.namedModule -> JSON.stringify(dependency.namedModule)

sokra · 2017-03-31T00:39:11Z

lib/dependencies/AMDDefineDependency.js

-				`!(__WEBPACK_AMD_DEFINE_FACTORY__ = (#), XXX = (typeof __WEBPACK_AMD_DEFINE_FACTORY__ === 'function' ?
-				(__WEBPACK_AMD_DEFINE_FACTORY__.call(exports, __webpack_require__, exports, module)) : __WEBPACK_AMD_DEFINE_FACTORY__))`
+				"var XXX,XXXfactory,XXXmodule;",
+				"!(XXXfactory = (#),(XXXmodule = { id: 'YYY', exports: {}, loaded: false }),XXX = (typeof XXXfactory === 'function' ? (XXXfactory.call(XXXmodule.exports, __webpack_require__, XXXmodule.exports, XXXmodule)) : XXXfactory),(XXXmodule.loaded = true),XXX === undefined && (XXX = XXXmodule.exports))"


'YYY' -> YYY

sokra · 2017-03-31T00:39:48Z

lib/dependencies/AMDDefineDependency.js

-				`!(__WEBPACK_AMD_DEFINE_FACTORY__ = (#), XXX = (typeof __WEBPACK_AMD_DEFINE_FACTORY__ === 'function' ?
-				(__WEBPACK_AMD_DEFINE_FACTORY__.call(exports, __webpack_require__, exports, module)) : __WEBPACK_AMD_DEFINE_FACTORY__))`
+				"var XXX,XXXfactory,XXXmodule;",
+				"!(XXXfactory = (#),(XXXmodule = { id: 'YYY', exports: {}, loaded: false }),XXX = (typeof XXXfactory === 'function' ? (XXXfactory.call(XXXmodule.exports, __webpack_require__, XXXmodule.exports, XXXmodule)) : XXXfactory),(XXXmodule.loaded = true),XXX === undefined && (XXX = XXXmodule.exports))"


Insert a single space after each ,
like above

webpack-bot · 2017-04-02T10:45:12Z

@zastavnitskiy Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

zastavnitskiy · 2017-04-02T10:54:40Z

Hey, I updated my forked repo and pushed requested changes. Now I also see commits from the main repo in the pull request. Do you know how to fix it?

webpack-bot · 2017-04-03T09:32:10Z

The most important CI builds succeeded. Great work so far.

webpack-bot · 2017-04-03T09:32:11Z

It looks like this Pull Request doesn't include enough test cases.

@zastavnitskiy Please add more test cases.

See test readme for details how to write test cases.

sokra · 2017-04-03T09:39:14Z

ahh.. I accidentally destroyed it...

webpack-bot · 2017-04-03T20:21:35Z

Hi @zastavnitskiy.

First thanks for your pull request.

Just a little hint from a friendly bot about the best practice when submitting pull request:

Don't submit pull request from your own master branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

zastavnitskiy mentioned this pull request Mar 21, 2017

Commonjs wrapped local modules are not resolved. #4511

Closed

sokra requested changes Mar 21, 2017

View reviewed changes

zastavnitskiy commented Mar 22, 2017

View reviewed changes

sokra requested changes Mar 23, 2017

View reviewed changes

webpack-bot added the PR: CI-ok label Mar 30, 2017

webpack-bot added PR: review-outdated labels Mar 30, 2017

sokra requested changes Mar 31, 2017

View reviewed changes

webpack-bot added PR: reviewed PR: review-outdated and removed PR: review-outdated PR: reviewed labels Mar 31, 2017

webpack-bot added the PR: tests-needed label Apr 3, 2017

sokra closed this Apr 3, 2017

sokra force-pushed the master branch from fa3ac29 to 4153b58 Compare April 3, 2017 09:37

sokra mentioned this pull request Apr 3, 2017

Bugfix/local amd commonjs wrapped #4626

Merged

webpack-bot added the PR: small label Apr 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix resolving of local common js wrapped modules. #4529

Fix resolving of local common js wrapped modules. #4529

zastavnitskiy commented Mar 21, 2017

sokra Mar 21, 2017

sokra Mar 21, 2017

sokra Mar 21, 2017

zastavnitskiy Mar 22, 2017 •

edited

sokra left a comment

zastavnitskiy left a comment

sokra left a comment

sokra Mar 23, 2017

zastavnitskiy Mar 28, 2017

sokra commented Mar 23, 2017

webpack-bot commented Mar 30, 2017

sokra Mar 31, 2017

zastavnitskiy Apr 2, 2017

sokra Mar 31, 2017

sokra Mar 31, 2017

sokra Mar 31, 2017

sokra Mar 31, 2017

webpack-bot commented Apr 2, 2017

zastavnitskiy commented Apr 2, 2017

webpack-bot commented Apr 3, 2017

webpack-bot commented Apr 3, 2017

sokra commented Apr 3, 2017

webpack-bot commented Apr 3, 2017

Fix resolving of local common js wrapped modules. #4529

Fix resolving of local common js wrapped modules. #4529

Conversation

zastavnitskiy commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zastavnitskiy Mar 22, 2017 • edited

Choose a reason for hiding this comment

sokra left a comment

Choose a reason for hiding this comment

zastavnitskiy left a comment

Choose a reason for hiding this comment

sokra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sokra commented Mar 23, 2017

webpack-bot commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Apr 2, 2017

zastavnitskiy commented Apr 2, 2017

webpack-bot commented Apr 3, 2017

webpack-bot commented Apr 3, 2017

sokra commented Apr 3, 2017

webpack-bot commented Apr 3, 2017

zastavnitskiy Mar 22, 2017 •

edited