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

require.resolveWeak does not have the same static resolution capabilities as import, require, etc #4993

Closed
faceyspacey opened this issue Jun 3, 2017 · 14 comments

Comments

@faceyspacey
Copy link
Contributor

faceyspacey commented Jun 3, 2017

Do you want to request a feature or report a bug?
BUG

What is the current behavior?
require.resolveWeak() can't statically determine module string passed to it as well as import().then.

modules[moduleId] is undefined here:

modules[moduleId].call(module.exports, module, module.exports, hotCreateRequire(moduleId));

the moduleId is "./src/components recursive ^\.\/.*$"

Here's the trace:

TypeError: Cannot read property 'call' of undefined
__webpack_require__
http://localhost:3000/static/bootstrap.js:694:29
fn
http://localhost:3000/static/bootstrap.js:115:20
resolve
http://localhost:3000/static/main.js:35453:34
requireSync
http://localhost:3000/static/main.js:80:58
webpackJsonp.../async-module/src/index.js.exports.default
http://localhost:3000/static/main.js:165:10
Loadable
http://localhost:3000/static/main.js:4176:47
getComponent
http://localhost:3000/static/main.js:35434:35
new App
http://localhost:3000/static/main.js:35379:16
http://localhost:3000/static/main.js:12070:18
measureLifeCyclePerf
http://localhost:3000/static/main.js:11850:12

If the current behavior is a bug, please provide the steps to reproduce.
For example, try:

require.resolveWeak(`${+new Date() % 2 ? './Example' : './Loading'}`)

What is the expected behavior?
The expected behavior is that resolveWeak will return the moduleId of the corresponding module. The weird thing is that it works when using Webpack on the server, but in the browser (Chrome) it does not work.

In addition, the following works:

import(/* webpackChunkName: "[request]" */ `${+new Date() % 2 ? './Example' : './Loading'}`).then(...

If this is a feature request, what is motivation or use case for changing the behavior?
The motivation and use case for this is for tools like React Loadable and React Universal Component which allow for async-loading + server-side rendering of the same component. Currently such tools don't allow for dynamic component selection (because it depends on resolveWeak for synchronous loading if the module is already available).

If it was just a case of using import().then, such expressions would work. But resolveWeak is needed to make the server-side synchronous rendering magic happen, as well as the synchronous rendering on initial load in the browser if the corresponding chunks are embedded before main.

This is a very important feature for the React Loadable and React Universal Component community and any other future async packages.

Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.

VERSION: 2.6.1 + 3.0.0
NODE: 7.7.2
OSX

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jun 3, 2017

Here's another example:

const name = foo ? 'Example' : 'Loading'
import(/* webpackChunkName: "[request]" */ `./${name}`))
require.resolveWeak(`./${name}`)

The transpiled code is:

var name = foo ? 'Example' : 'Loading';
__webpack_require__("./src/components lazy recursive ^\\.\\/.*$")("./" + name)
/*require.resolve*/(__webpack_require__("./src/components recursive ^\\.\\/.*$").resolve("./" + name))

The differences are the word "lazy" and the fact that resolveWeak has .resolve at the end. It just seems like it should have the same capabilities. All resolveWeak is supposed to do is not declare a dependency, but the static interpretation should still function the same.

It seems possibly the new lazy feature which import async has may be the difference here:

https://webpack.js.org/guides/code-splitting-async/#chunk-names

Is there an updated version of webpack.resolveWeak to reflect some of this new stuff?

I even tried the following and it didn't work: require.resolveWeak(/* webpackMode: "lazy" */ ./${name})

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jun 3, 2017

Also, to be clear, I have my split bundles and bootstrap code properly setup, eg:

<script type='text/javascript' src='/static/bootstrap.js'></script>
<script type='text/javascript' src='/static/0.js'></script>
<script type='text/javascript' src='/static/main.js'></script>

So import() and resolveWeak is called in main.js, and the split chunk is properly evaluated before it, but after the webpack bootstrap code. Everything works perfectly, as long as I'm not constructing dynamic paths.

The fact that it works server-side where the LimitChunkCountPlugin creates one 1 chunk leads me to think this has potential to work client-side too. If I didn't have my 0.js bundle before my main.js bundle and the bootstrap code properly setup before everything, I could understand why this is not working. But the way I have it setup is basically the same thing as what's running on the server. Unless there is something I'm missing??

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jun 8, 2017

@sokra

primarily because of all your hardwork on Webpack these last few years, I was able to bring the code-splitting [+ SSR] thing full circle:

https://medium.com/@faceyspacey/code-cracked-for-code-splitting-ssr-in-reactlandia-react-loadable-webpack-flush-chunks-and-1a6b0112a8b8

check it out. I put your magic comments to use as part of a complete server-side flushing system. I hope you like it and approve. I was aiming to come up with the most idiomatic solution.


Also, if and when this issue is fixed I plan on releasing another article about using HoCs with universal components + universal rendering. See, with this issue fixed, we can now have dynamic universal components, not just one. That's a very important piece to this puzzle. In fact, it's the final piece.

ps. I'm also the guy behind the Extract Css Chunks Webpack Plugin:

https://github.com/faceyspacey/extract-css-chunks-webpack-plugin

I know in the past u didn't like HMR with that plugin. But the thing is this is all part of the code-splitting + SSR dream. This all needs to be frictionless. There's no issues with my fork, except a warning I'd like to get rid of. Because of the way HMR works, the first change u make always gives a warning that might lead u to think HMR won't work. It in fact does, but the warning is disconcerting. I'm sure it's because of the way I hacked this together, which is why its creator really should be involved to make the official version. I can help. But basically after looking at what I've done u'll probably know exactly what to do.

Anyway, it's a new future. Code Splitting + SSR is complete. Thank you.

@jonathancalb
Copy link

I found this issue breaks silently my application giving me "TypeError: Cannot read property 'call' of undefined" when I try to reach one of my react application routes. If I don't try to navigate to a route it works ok. For sure when I navigate to a route it uses resolveWeak and there it breaks. I found that if I disable CommonsChunkPlugin it work ok so I think it has something to be with that. Hope my experience helps to push this forward. It started to happen like a week ago or so, maybe with some dependency update. I use Webpack 2; Any other information I can give to help just ask me.

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jun 18, 2017

this bug and #4812 are duplicates FYI.

@sokra ? it seemed before, u planned not to make resolveWeak have this capability or perhaps thought it was not possible. Does the given use case with react-loadable, react-universal-component, etc make it possible? ;)

I sure hope so. Or at least a suggested alternative for the use case of a synchronous fallback.

@faceyspacey
Copy link
Contributor Author

I've been diving into the Webpack code and seeing if I can fix it. It's no doubt an intimidating codebase to start with. The location I've come up with is this:

const comment = outputOptions.pathinfo ?

It seems because there is no dep.range, resolve weak isn't getting the proper comment here:

/*require.resolve*/(__webpack_require__("./src/components recursive ^\\.\\/.*$").resolve("./" + name))

i.e. it's missing the lazy as in a regular require:

__webpack_require__("./src/components lazy recursive ^\\.\\/.*$")("./" + name)

Obviously there is more it, as it does receive some sort of comment.. And it also gets the require.resolve comment ahead of time. So perhaps it's being parsed somewhere else, and it needs the comment there. I'm not sure if adding lazy just makes it work. But perhaps if I can get some direction, I can keep digging and figure the rest out.

@TheLarkInn any word? I'm not sure if my help is worth a damn, but I'm more than willing to try. A few words from somebody who knows how this stuff works could go a long way.

@faceyspacey
Copy link
Contributor Author

actually my bad, it's not a comment it's meant to receive. But it seems like it's around here where resolveWeak misses out in some of the capabilities of a regular require or resolve.

@faceyspacey
Copy link
Contributor Author

the culprit file seems to be this one in fact:

https://github.com/webpack/webpack/blob/93ac8e9c3699bf704068eaccaceec57b3dd45a14/lib/dependencies/ContextDependencyTemplateAsId.js

basically the files in the lib/dependencies directories with the words context and import and AsId in the file titles seem to be the relevant files.

@faceyspacey
Copy link
Contributor Author

const dep = ContextDependencyHelpers.create(RequireResolveContextDependency, param.range, param, expr, options);

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jul 6, 2017

After debugging the Webpack compiler, the issue is that the require.resolveWeak expression is treated as NormalModule rather than a ContextModule like import().

For example, import() passes through here:
https://github.com/webpack/webpack/blob/master/lib/ContextModule.js#L312

but resolveWeak should go through the following but doesn't:
https://github.com/webpack/webpack/blob/master/lib/ContextModule.js#L329

If it did, the resolve function would exist as expected:
https://github.com/webpack/webpack/blob/master/lib/ContextModule.js#L181

getSyncSource(dependencies, id) {
		const map = this.getUserRequestMap(dependencies);
		return `var map = ${JSON.stringify(map, null, "\t")};

  function webpackContext(req) {
	return __webpack_require__(webpackContextResolve(req));
  };

  function webpackContextResolve(req) {
	var id = map[req];
	if(!(id + 1)) // check for number or string
		throw new Error("Cannot find module '" + req + "'.");
	return id;
  };

  webpackContext.keys = function webpackContextKeys() {
	return Object.keys(map);
  };

  webpackContext.resolve = webpackContextResolve;
  module.exports = webpackContext;
  webpackContext.id = ${JSON.stringify(id)};`;
}

I'm trying to figure out now how to mark the expression to use the ContextModule. Any help would be much appreciated. Besides plugin/loader work, this is my first time delving into the real webpack codebase. I know ASTs and how to make Babel plugins, but the process of how all this works is new to me. Is there docs I should be looking at?

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jul 6, 2017

So basically it's about making resolveWeak able to use context, and just getting the exact same mapping of possible arguments to module IDs that import() gets. For example a concrete example of the transpiled code for an import(./async/${page}) context module is:

var map = {
	"./App": [
		34
	],
	"./App.js": [
		34
	],
	"./Example": [
		135,
		0
	],
	"./Example.js": [
		135,
		0
	]
};
function webpackAsyncContext(req) {
	var ids = map[req];
	if(!ids)
		return Promise.reject(new Error("Cannot find module '" + req + "'."));
	return Promise.all(ids.slice(1).map(__webpack_require__.e)).then(function() {
		return __webpack_require__(ids[0]);
	});
};
webpackAsyncContext.keys = function webpackAsyncContextKeys() {
	return Object.keys(map);
};
module.exports = webpackAsyncContext;
webpackAsyncContext.id = 372;

/***/ }),

So it's not like it can't easily be done.

For a resolveWeak context module, the result code would have a resolve function like this:

var map = {
	"./App":  34,
	"./App.js": 34,
	"./Example": 135,
	"./Example.js": 135,
}

module.resolve = function(key) {
	return map[key]
};

@faceyspacey
Copy link
Contributor Author

the latest thinking is to create the equivalent of require.contextAsync. The reasoning being that require.context('./' false, /\.js$/) for example generates exactly what we need:

/***/ }),
/* 370 */
/***/ (function(module, exports, __webpack_require__) {

var map = {
	"./Example.js": 78,
	"./Foo.js": 79
};
function webpackContext(req) {
	return __webpack_require__(webpackContextResolve(req));
};
function webpackContextResolve(req) {
	var id = map[req];
	if(!(id + 1)) // check for number or string
		throw new Error("Cannot find module '" + req + "'.");
	return id;
};
webpackContext.keys = function webpackContextKeys() {
	return Object.keys(map);
};
webpackContext.resolve = webpackContextResolve;
module.exports = webpackContext;
webpackContext.id = 370;

That's what getSyncSource is used for. So basically if we can create the above without marking the contained modules as dependencies, we win. Currently by virtue of creating the context, all the children are bundled into the parent which is exactly what we don't want.

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Jul 7, 2017

Ok I finally figured it out. Basically, in lib/ContextModule.js we create a weak-context async mode and do similar to what is done when this.async === 'lazy', but without adding the dependencies to the parent bundle:

if(this.async === "weak-context") {

  // usually this assigns to this.dependencies, but that would actually bundle them
  this.weakDependencies = weakDependencies;

  // copied from "lazy" async mode:
  dependencies.forEach((dep, idx) => {
    let chunkName = this.chunkName;

    if(chunkName) {
      if(!/\[(index|request)\]/.test(chunkName))
        chunkName += "[index]";
      chunkName = chunkName.replace(/\[index\]/g, idx);
      chunkName = chunkName.replace(/\[request\]/g, Template.toPath(dep.userRequest));
    }
    const block = new AsyncDependenciesBlock(chunkName, dep.module, dep.loc);
    block.addDependency(dep);
    this.addBlock(block);
  });

} else if(this.async === "lazy-once") {...

...then when it comes to getting the source string we do this:

getSourceString(asyncMode, outputOptions, requestShortener) {
  if(asyncMode === 'weak-context') {
     return this.getSyncSource(this.weakDependencies, this.id);
  }
  else if(asyncMode === "lazy") {...

and we also gotta parse the arguments:

module.exports = class RequireContextDependencyParserPlugin {
 apply(parser) {
  parser.plugin("call require.context", expr => {
   let regExp = /^\.\/.*$/;
   let recursive = true;
   let weak = false
   let chunkName
   switch(expr.arguments.length) {
    case 5:
    {
      const chunkNameExpr = parser.evaluateExpression(expr.arguments[4]);
      if(!chunkNameExpr.isString()) return;
      chunkName = chunkNameExpr.string;
    }
    case 4:
    {
      const weakExpr = parser.evaluateExpression(expr.arguments[3]);
      if(!weakExpr.isBoolean()) return;
      weak = weakExpr.bool;
    }
    case 3:
    {
      const regExpExpr = parser.evaluateExpression(expr.arguments[2]);
      if(!regExpExpr.isRegExp()) return;
      regExp = regExpExpr.regExp;
    }
    // falls through
    case 2:
    {
      const recursiveExpr = parser.evaluateExpression(expr.arguments[1]);
      if(!recursiveExpr.isBoolean()) return;
      recursive = recursiveExpr.bool;
    }
    // falls through
    case 1:
    {
      const requestExpr = parser.evaluateExpression(expr.arguments[0]);
      if(!requestExpr.isString()) return;
      const dep = new RequireContextDependency(requestExpr.string, recursive, regExp, chunkName, expr.range);
      dep.loc = expr.loc;
      dep.optional = parser.scope.inTry;
      parser.state.current.addDependency(dep);
      return true;
    }
   }
  });
 }
};

and:

class RequireContextDependency extends ContextDependency {
	constructor(request, recursive, regExp, weak, chunkName, range) {
		super(request, recursive, regExp);
		this.range = range;

		if (weak) {
			this.async = 'weak-context'
			this.chunkName = chunkName
		}
	}

	get type() {
		return "require.context";
	}
}

...So that allows us to create a weak context today via:

require.context('./asyncComponents', true, /^\.\/.*$/, true, 'asyncComponents/[request]')

or something like that, with the final option indicating to create a weak context. Perhaps in the end it's another function like require.contextWeak.

Either way, that basically solves the problem. If we want to go further, we make the template for resolveWeak implement a weak context. Those are my next steps.

@faceyspacey
Copy link
Contributor Author

And here's the PR:

#5235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants