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

Server doesn't work with esm package #455

Closed
brandon-pereira opened this issue Oct 28, 2019 · 5 comments · Fixed by #457
Closed

Server doesn't work with esm package #455

brandon-pereira opened this issue Oct 28, 2019 · 5 comments · Fixed by #457

Comments

@brandon-pereira
Copy link
Contributor

brandon-pereira commented Oct 28, 2019

🐛 Bug Report

When using @loadable/server with the esm package, when you instantiate the ChunkExtractor you will get the following error:

TypeError: Cannot read property 'filename' of undefined
    at /Users/brandonp/Sites/myrepo/node_modules/esm/esm.js:1:252056
    at smartRequire (/Users/brandonp/Sites/myrepo/node_modules/@loadable/server/lib/util.js:23:32)
    at new ChunkExtractor (/Users/brandonp/Sites/myrepo/node_modules/@loadable/server/lib/ChunkExtractor.js:173:50)
    at renderApp (/Users/brandonp/Sites/myrepo/src/backend/renderApp.js:63:29)
    at process._tickCallback (internal/process/next_tick.js:68:7)

To Reproduce

Steps to reproduce the behavior:

Run the following with a valid nodeStats path:

      const nodeExtractor = new ChunkExtractor({
        statsFile: nodeStats,
        entrypoints: ['main']
      });

Expected behavior

ChunkExtractor successfully loads and uses the required nodeStats file path.

 const nodeStats = path.resolve('./dist/backend/app/loadable-stats.json');`

The fix (IMO)

You're currently using module.require. If I tweak this line to read as follows:

  try {
    // This will fail with 'esm' because module not accessible
    return eval('module.require')(modulePath);
  } catch (err) {
    // Fallback on normal require
    return require(modulePath);
  }

More about why module.require is not defined here.

I am more than happy to open a PR for this, but unsure if this is the solution you want to take. Please let me know!

Run npx envinfo --system --binaries --npmPackages @loadable/component,@loadable/server,@loadable/webpack-plugin,@loadable/babel-plugin --markdown --clipboard

Paste the results here:

## System:
 - OS: macOS Mojave 10.14.6
 - CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
 - Memory: 966.62 MB / 16.00 GB
 - Shell: 5.3 - /bin/zsh
## Binaries:
 - Node: 10.15.1 - ~/.nvm/versions/node/v10.15.1/bin/node
 - Yarn: 1.17.3 - /usr/local/bin/yarn
 - npm: 6.4.1 - ~/.nvm/versions/node/v10.15.1/bin/npm
## npmPackages:
 - @loadable/babel-plugin: ^5.10.3 => 5.10.3 
 - @loadable/component: ^5.10.3 => 5.10.3 
 - @loadable/server: ^5.10.3 => 5.10.3 
 - @loadable/webpack-plugin: ^5.7.1 => 5.7.1 
@brandon-pereira brandon-pereira changed the title Doesn't work with esm package Server doesn't work with esm package Oct 28, 2019
@theKashey
Copy link
Collaborator

According to the nodejs documentation module.require is needed only to require some module on behalf of another module, thus it should be or

  • just require
  • or the function should accept module as well.

PS: using module as a keyword disables webpack concatenation - it has to keep the module as a separated entity.

PPS: All require-s should be invisible for the bundler, ie wrapped with eval

@brandon-pereira
Copy link
Contributor Author

Should I update the code to require by default unless a useModule boolean is present? Ie

     // uses require
      const nodeExtractor = new ChunkExtractor({
        statsFile: nodeStats,
        entrypoints: ['main']
      });

     // use module.require
      const nodeExtractor = new ChunkExtractor({
        statsFile: nodeStats,
        useModuleRequire: true,
        entrypoints: ['main']
      });

Obviously that name is terrible :P Also.. this would result in a potentially breaking change? Thats what I was trying to avoid

@theKashey
Copy link
Collaborator

Look like smartRequire is used only to load stats.json without importing them to the main bundle, if SSR code is by some reason used for both cases.
There is no need to track the "real" owner module, so it's ok to use just require. However I don't know why @neoziro used it initially - probably webpack is smart enough to handle eval('require'), however foolwebpack is ok with it.

@brandon-pereira
Copy link
Contributor Author

Okay, I can switch it to using a simple require. Should I add foolwebpack as a dependency or should I just copy the logic they use to require? Not sure if you want another dependency added.

@brandon-pereira
Copy link
Contributor Author

@theKashey PR opened which changes eval('module.require') to eval('require'). @neoziro if you have any insights as to why module.require was used I'd be interested to hear!

Let me know if I can change/improve anything else, happy to help. Thanks for making this package :)

gregberge pushed a commit that referenced this issue Dec 2, 2019
this improves support with 'esm' package

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

Successfully merging a pull request may close this issue.

2 participants