themgoncalves / react-loadable-ssr-addon

Server Side Render add-on for React Loadable. Load splitted chunks was never that easy.
https://themgoncalves.gitbook.io/react-loadable-ssr-aadon/
MIT License
69 stars 19 forks source link

Cannot distinguish between two modules with same relative name #5

Closed tomkelsey closed 6 years ago

tomkelsey commented 6 years ago

Expected Behavior

In folder1: import('./Content')

In folder2: import('./Content')

The two Content components are different so should be seen as separate files despite having the same relative name.

Current Behavior

The two Content components are seen as the same.

Possible Solution

Someone created a PR on react-loadable with a suggestion for a fix: https://github.com/jamiebuilds/react-loadable/pull/111

Steps to Reproduce

As above under Expected Behaviour

Other Comments

Thanks for the quick resolution of my other issue - apologies for creating another so soon after!

themgoncalves commented 6 years ago

Thanks for reporting @tomkelsey!

Understanding the issue

This issue happens because react-loadable uses the chunk origin path (file import path) as key, and as you can see, this is not a unique key, which means that if you are importing two distinct files but with the same path (relative or absolute), it will generate this error.

Example:

Lets take a look in this folder structure:

source
├── components
│   ├── header
│   │   ├── index.js               # Place where we will dynamically import the component bellow
│   │   │   ├── myComponentToBeDynamicallyImported.js
│   ├── menu
│   │   ├── index.js               # Place where we will dynamically import the component bellow
│   │   │   ├── myComponentToBeDynamicallyImported.js 

See that we have two components with the same name, myComponentToBeDynamicallyImported, although it is in different folders, it can be dynamically imported with the same path.

E.g. import('./myComponentToBeDynamicImported')

By that, react-loadable would get its path and use as key -> ./myComponentToBeDynamicallyImported, which is the reason of this issue: duplicated key.

Sadly, the fix for it has to come from react-loadable due to we have to use the same key as they to achieve the results from react-loadable-ssr-addon.

Suggested fix

While react-loadable won't fix this issue, the fix for this is quite simple, we just have to set a unique name for the dynamically imported files.

In this case, you can customise its name using the following Webpack synthax:

import(/* webpackChunkName: "MenuDynamicImport" */ './myComponentToBeDynamicallyImported')

and

import(/* webpackChunkName: "HeaderDynamicImport" */ './myComponentToBeDynamicallyImported').

By which will generate two different files and as well, keys.

Note that you will need to configure Webpack settings as well to accept this:

module.exports = {
  ...
  output: {
    ...
    filename: '[name].bundle.js',
    chunkFilename: '[name].chunk.js',      // <- this is the required configuration
  },
  ...

Also, you build output will have a batter readability!

For more informations, check the Webpack Code Splitting Documentation.

tomkelsey commented 6 years ago

Thank you @themgoncalves for such an extensive reply and workaround. Much appreciated!

tomkelsey commented 6 years ago

Just following up on your suggested fix: I believe it may cause an issue when you import the same component twice from different relative locations?

For example:

import(/* webpackChunkName: "SameComponent" */ './theSameComponent')

then elsewhere in the project:

import(/* webpackChunkName: "SameComponent" */ '../../theSameComponent')

In the generated assets file, I believe they'll only be one entry in origins for './theSameComponent' and not '../../theSameComponent'

And so when rendering on the server for '../../theSameComponent' it looks up the key, cannot find it, and therefore does not return the required assets?

tomkelsey commented 6 years ago

Just wondering if something like this might work:

  getChunkOrigins(chunk) {
    const { modules, id, names } = chunk;
    const origins = new Set();
    for (let i = 0; i < modules.length; i += 1) {
      const { reasons } = modules[i];
      for (let j = 0; j < reasons.length; j += 1) {
        const { type, userRequest } = reasons[j];
        if (type === 'import()') {
          origins.add(userRequest);
        }
      }
    }

    if (origins.size === 0) {
      return [names[0] || id];
    }

    return Array.from(origins);
  }
  getAssets(assetsChunk) {
    for (let i = 0; i < assetsChunk.length; i += 1) {
      const chunk = assetsChunk[i];
      const { id, files, siblings = [], hash } = chunk;

      const keys = this.getChunkOrigins(chunk);
      keys.forEach(key => {
        this.assetsByName.set(key, {
          id,
          files,
          hash,
          siblings
        });
      });
    }

    return this.assetsByName;
  }
themgoncalves commented 6 years ago

@tomkelsey I will have to do some investigation here around your report and get back to you later!

Thanks for your collaboration so far 🤟

themgoncalves commented 6 years ago

@tomkelsey your solution worked like a charm! It was released in the v0.1.4, by which you can update you package right now.

Have a nice day.