Doesn't work with "multiple files" import

[fatfisz]

Describe the bug
This code causes the "handle-import" babel plugin to break:

import(
  /* webpackInclude: /\.docs\.js$/, webpackChunkName: "whatever-[request]" */
  `__cwd/${path}`,
)

To Reproduce

1. Include the snippet above in a code.
2. Receive Module build failed: TypeError: Cannot read property '0' of undefined error from node_modules\next\dist\server\build\babel\plugins\handle-import.js:35:30

Expected behavior
I'd expect Next.js to be able to handle this kind of import usage, as it's supported by webpack. The old way to do the same, namely require.context, is not breaking like this (though it's breaking in other ways, as I'll bescribe in "additional context").

System information

• OS: Windows 10
• Version of Next.js: 6.0.3

Additional context
Related issue: #2690

This problem is directly caused by the "handle-import" plugin expecting the first argument to be a string literal (which has the value property). Since in my case it's a template literal, it doesn't have a value, only expressions and quasis.

---

For a while now I've been trying to create a wrapper around Next.js that's able to work with dynamically imported files (a tool for documentation which includes all *.docs.js files in the project).

First of all, I can't use pages/ - I want documentation files to be close to the scripts they document. I want the original structure of the source to be reflected by the structure of documentation, so replicating the directory structure in two places (original and pages) seems like an unmaintainable idea.

That's how I arrived at a solution that uses import-all.macro. The problem with that is, it's powered by babel. So when, as a user, I'm adding a new documentation file, I'll need to stop the tool, purge the cache (since all imports are inlined in a file), and restart the tool. I'm losing the file-listening capabilities of webpack-dev-server, and I need to go around the limitations of babel caching.

That's how I arrived at a solution that uses require.context. It almost works! I can't get the SSR story correctly though - while the server renders the appropriate script, the chunk doesn't seem to be ready when dynamic is rendering the component. Actually, the chunk doesn't seem to be generated at all! That's why I switched to import, to try and see if I could get the chunk-creating machine going at least with the other way of importing. As it is now, I've spent several days on this, I'm spinning in place, and I'm determined to fix the problem.

[fatfisz]

There seems to be an existing solution: https://github.com/faceyspacey/react-universal-component together with https://github.com/faceyspacey/webpack-flush-chunks. I'll see if I can get them working with Next.js.

If they work alright would it be a viable option to replace next/dynamic with react-universal-component in Next.js? Or is it in the domain of Next.js plugins?

[fatfisz]

I managed to make everything work. I'm very excited, because for so many days I was hoping that "the next solution will finally work" and there was always something lacking. But to get to the final solution, I had to change Next.js in a few ways - I'd like to get those changes into the package, as I think they'll benefit not only me, but also Next.js in the long run.

TL;DR:

• Fixing the condition in handle-import to ignore template literals is very important (the current behavior of just crashing with an unhelpful error message is very undesirable)
• Fixing getAvailableChunks to remove only the last hyphen and what follows it is also very important (it's because of how Webpack constructs chunk names - I can't do much here)
• Upgrading to Webpack 4 would help a tiny bit

Below is a comprehensive description of whats and whys.

The handle-import plugin

As far as I know, the only way to set chunk names for "context" imports is to use this construct:

import(
	/* "webpackChunkName": "chunks/[request]" */
	`__cwd/${path}.docs.js`,
)

__cwd is resolved to process.cwd(), which is not the same directory where Next.js is running from

This is quite inconvenient because of three things:

• It's impossible to get a map of all paths from this, as I described here: Add require.context.keys-like feature to import webpack/webpack#7283
• Because of that I have to retrieve the map of paths using require.context('__cwd', true, /\.docs\.js/, 'weak');, which forces me to use a hack (I'll describe it below)
• Webpack docs describe syntax that's only available in v. 4, so I wasted some time on discovering that webpackChunkName has to be put in quotes in v. 3 (otherwise this just breaks and the error message is not helpful)

What I'd benefit here from (a bit) is to have Webpack upgraded to v. 4, but that's a small issue.

As an extra thing, I have to actually use something like this:

const marker = '|';
import(
	/* "webpackChunkName": "chunks/[request]" */
	`__cwd/${marker}${path}${marker}.docs.js`,
)

Here I'm using the fact that Webpack doesn't know what marker is at the build time, so it just looks up the same paths as before. Afterwards though, I can easily get the actual path by getting the one between the markers (the ${path} argument comes from require.context, which returns full paths, so the result might look something like this: ././path/to/file.docs.js.docs.js, and after marking it's ./|./path/to/file.docs.js|.docs.js).

Ok, but at this point everything's still failing, because of the bug in server/build/babel/plugins/handle-import.js. What I did to fix the situation was to change the first condition inside CallExpression visitor to:

if (path.node.callee.type === TYPE_IMPORT && path.node.arguments[0].type !== 'TemplateLiteral') {
  ...
}

This just makes the plugin ignore template literals. I'd like it very much to see this change added to Next.js soon - if Next.js is not doing anything meaningful with import(`foo${bar}`), then it could at least not explode.

The getAvailableChunks utility function

So at this point I've added a few things here and there to be able to import files dynamicly like a civilised human. I got to know a bit about Webpack internals, and I've added an extension that not only allows me to get the chunk name for any file in import(`foo${bar}`), but also sets the proper value of module.__webpackChunkName so that Next.js knows such and such chunk was used when rendering a page. I don't really need this to be added to Next.js, but I'm leaving this here; maybe it will be useful someday when context imports come to Next.js:

The big hack that solves all my chunk-mapping-related problems

const marker = JSON.stringify('|');
const getExtraLazy = chunkNameMap => `
var chunkNameMap = ${JSON.stringify(chunkNameMap, null, '\t')};

function webpackContextResolveChunk(req) {
  if (!map.hasOwnProperty(req)) {
    throw new Error("Cannot find module '" + req + "'.");
  }
  return chunkNameMap[map[req][1]];
};

function extraWebpackAsyncContext(req) {
  if (req.indexOf(${marker}) === -1) {
    throw new Error("Cannot handle unmarked module request '" + req + "'.");
  }

  var unmarkedReq = req.slice(req.indexOf(${marker}) + 1, req.lastIndexOf(${marker}));

  return webpackAsyncContext(unmarkedReq).then(function(module) {
    var chunkId = webpackContextResolveChunk(unmarkedReq);
    module.__webpackChunkName = chunkId;
    return module;
  })
};

module.exports = extraWebpackAsyncContext;
`;

function getChunkNameMap(module) {
  return fromPairs(flatten(
    module.blocks
      .filter(block => block.dependencies[0].module)
      .map(block => (block.chunks || []).map(chunk => [chunk.id, chunk.name.slice('chunks/'.length)])),
  ));
}

class ExtraLazyTemplatePlugin {
  apply(moduleTemplate) {
    moduleTemplate.plugin('module', (source, module) => {
      if (module.async !== 'lazy') {
        return source;
      }
      const chunkNameMap = getChunkNameMap(module);
      return new ConcatSource(source, getExtraLazy(chunkNameMap));
    });
  }
}

class ExtraLazyPlugin {
  apply(compiler) {
    compiler.plugin('compilation', compilation => {
      compilation.moduleTemplate.apply(new ExtraLazyTemplatePlugin());
    });
  }
}

I'm almost completely set, but I still see a flash of "Loading..." after opening the page (proper markup is sent because of SSR, then "Loading..." appears, then the page is rendered again). And the reason for that is that my chunks are still being ignored by Next.js. I tracked it down to the following line in getAvailableChunks:

const chunkName = filename.replace(/-.*/, '')

If I understand correctly, the goal of this line is to strip the hash preceeded by a hyphen. Unfortunately with my chunks named chunks/[request], I get a lot of hyphens in the name - Webpack changes all slashes to hyphens. But because we're interested only in the last hyphen and what comes after it, my suggested change is this:

const chunkName = filename.replace(/-[^-]*$/, '');

It correctly strips the file name of the last hyphen and the remaining part.

---

Sorry if the write up is too long. It was a long ride for me after all, and I needed to get this off my chest. My biggest hope right now is that maintainers will agree with two small fixes I suggested. I'm here to help and I'd gladly prepare the needed PRs.

[fatfisz]

@timneutkens I hope I'm not overstepping my bounds if I ask you to give me any feedback on this? Right now I could prepare PRs for changes that I suggested - I just want to make sure that the problems I highlighted are something that you'd also like to see fixed.

[timneutkens]

@fatfisz let me read up on this soon! it's quite a lot of research 😄, I've been thinking about making some changes to next/dynamic anyway 👍

[fatfisz]

Hi, could I just create said PRs? They'll be very small and easy to merge - I promise 🙂

I'd like to move forward with my project and while I can patch webpack to do what I need, Next.js is much harder to adjust (I'd need to release my own version, monkey patching is impossible).

I'm sure that those changes won't affect Next.js negatively regardless of what direction next/dynamic will take.

[timneutkens]

Sure go ahead with creating a PR, I can review them and we have extensive tests for the feature anyway, so if you break something you'll know 😄

[fatfisz]

Just a heads up that both branches seem to be passing. I just updated them with the latest canary.

[dharmilsan]

Looks like some of the changes that @fatfisz suggested have been merged. But dynamic imports (using dynamic paths) still don't work with NextJS v6.1.1

[timneutkens]

Will be fixed with #4639

[fatfisz]

This sounds awesome - so you'll be dropping handle-import and using react-loadable instead?

[timneutkens]

@fatfisz yep that's what I'm working on, works great so far 👍

[briankoey]

Replace the line with require(variable/template string), what is the consequence of doing this?