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

Multiple entry points not working correctly #88

Closed
michaelgmiller1 opened this issue May 12, 2020 · 20 comments · Fixed by #102
Closed

Multiple entry points not working correctly #88

michaelgmiller1 opened this issue May 12, 2020 · 20 comments · Fixed by #102

Comments

@michaelgmiller1
Copy link

michaelgmiller1 commented May 12, 2020

Our team has a config with the form:

{
  ...
  entry: {
    entry1: ['file1', 'file2'],
    entry2: ['file3', 'file4'],
  }
}

We were very excited to try out react-refresh-webpack-plugin, but unfortunately, could not get it working!

Through a process of trial and error, I removed entry2, and it started working. I think this is due to the logic in injectRefreshEntry, which would double-append react refresh into both entries. Is this intentional behavior?

Thanks!
Mike

@michaelgmiller1
Copy link
Author

michaelgmiller1 commented May 12, 2020

This is easy to reproduce with the following one-line change to the webpack-dev-server example:

diff --git a/examples/webpack-dev-server/webpack.config.js b/examples/webpack-dev-server/webpack.config.js
index 0422c13..a5cd4b2 100644
--- a/examples/webpack-dev-server/webpack.config.js
+++ b/examples/webpack-dev-server/webpack.config.js
@@ -8,6 +8,7 @@ module.exports = {
   mode: isDevelopment ? 'development' : 'production',
   entry: {
     main: './src/index.js',
+    test1: './src/ClassDefault.jsx',
   },
   module: {
     rules: [

@pmmmwh
Copy link
Owner

pmmmwh commented May 12, 2020

Through a process of trial and error, I removed entry2, and it started working. I think this is due to the logic in injectRefreshEntry, which would double-append react refresh into both entries. Is this intentional behavior?

It is intended - there is no way we can control the execution order of multiple entry points. I would love to help, but I cannot comment on your specific case unless I have more context.

This is easy to reproduce with the following one-line change to the webpack-dev-server example

This is not necessarily how multi-entry should be used to my understanding. If you're doing a single page app, you should use a single entry point and use optimization.splitChunks instead.

Referencing the Webpack docs here:

In webpack version < 4 it was common to add vendors as a separate entry point to compile it as a separate file (in combination with the CommonsChunkPlugin).

This is discouraged in webpack 4. Instead, the optimization.splitChunks option takes care of separating vendors and app modules and creating a separate file. Do not create an entry for vendors or other stuff that is not the starting point of execution.

As a rule of thumb: Use exactly one entry point for each HTML document.

I'll tinker with Webpack and see if there's anything I can do in this regard.

@michaelgmiller1
Copy link
Author

michaelgmiller1 commented May 12, 2020

Our use case is that we load Sentry (a JS error tracking framework) via a separate bundle. If we had the ability to exclude entry points from being injected with react-refresh, it would solve our issue.

Would you be OK with a pull request adding a new option to disable entries ignoreEntryList?

@gaearon
Copy link

gaearon commented May 12, 2020

What is the root of the actual issue? Why is it bad (from one point of view) or necessary (from another point of view) to inject it in all entries?

Every option makes configuring it more error-prone. Can we figure out a reasonable default behavior?

@michaelgmiller1
Copy link
Author

michaelgmiller1 commented May 12, 2020

Not too sure what the root issue is.. the runtime looks like it just calls Refresh.register in react-refresh/runtime. @pmmmwh do you have any ideas about why double injecting would break? or any other preferred solutions?

@pmmmwh
Copy link
Owner

pmmmwh commented May 12, 2020

What is the root of the actual issue?

In my preliminary tests it seems that the module.hot.accept handlers were not handled correctly across entries. It may or may not have something to do with chunking behaviour. I'll have to dig deeper to see which part of the link is broken though, so this might end up being non-fixable if we keep the current behaviour.

Why is it necessary to inject it in all entries?

The main issue with multiple entries is that there's no deterministic way for us to ensure that code will be executed in a specific order. For the case of react-refresh, we will need to get runtime code (which sets up $RefreshReg$ and $RefreshSig$) to run before any user code or a ReferenceError will occur.

This is partially an artifact from the webpack<4 era, when people used to use entries to do chunking and create async/parallel-loaded independent bundles. This is not really the case now - multiple entries are discouraged by Webpack officially for single-page apps and should be used only when you have multiple HTML entries.

CC @sokra - would love your thoughts on my take on this.

Can we figure out a reasonable default behavior?

If my interpretation above is correct, then the current behaviour is a reasonable default - we're ensuring that the refresh runtime will be ran for all independent HTML entry points at the start, and they won't intervene with each other.

Would you be OK with a pull request adding a new option to disable entries ignoreEntryList?

This will work, but I would want to try and see if we can solve this by other means before resorting to exposing more options here.

@gaearon
Copy link

gaearon commented May 12, 2020

Why does including the runtime multiple times cause a problem? Can we simply make its second execution a no-op? i.e. it would not override any globals and would not do any side effects if $RefreshReg$ is already set up.

@sokra
Copy link

sokra commented May 12, 2020

You can use multiple entry points on a single page, but make sure to use optimization.runtimeChunk: "single". This makes sure you only have a single runtime (with module cache) and modules are not instantiated twice.

Webpack will also add the ability to create entry points that are children of other entries with dependOn.

Anyway: Use a single webpack runtime per html page.

@sokra
Copy link

sokra commented May 12, 2020

I think it would be the best to avoid using globals and instead plugin into the parser to rewrite $RefreshSig$ etc to be local to the bundle/runtime. Otherwise you run into problems when using multiple webpack builds on a page, e.g. with microfrontends and module federation. Globals are bad anyway.

The webpack ApiPlugin has examples how to rewrite in the parser. It need to be rewritten e.g. to something like __webpack_require__.$RefreshSig$.

@michaelgmiller1
Copy link
Author

@pmmmwh Do you think this will be straightforward to implement? We can workaround on our side, debating whether we should do the workaround, or wait for an update on the plugin.

No worries either way! Just want to figure out what works best on our side.

@pmmmwh
Copy link
Owner

pmmmwh commented May 15, 2020

Do you think this will be straightforward to implement?

It will probably take some time (few days to 1-2 weeks) because the fix Tobias suggested will require some work to properly wire up Webpack parser hooks. If you have a work-around, feel free to do so.

@michaelgmiller1
Copy link
Author

we'll actually probably hold off until this issue is fixed! let me know if I can do anything to help.

@pmmmwh
Copy link
Owner

pmmmwh commented May 19, 2020

@sokra
Is there a proper way to inject an external dependency to Webpack's require function (i.e. __webpack_require__)? The main issue blocking me using parser rewrites instead of globals is that $RefreshSig$ is an external function (createSignatureFunctionForTransform from react-refresh/runtime) but I need that in global scope (during initialization in MainTemplate). I think even the implementation in ProvidePlugin wouldn't work because the main template won't get processed by the parser.

I was thinking to do something along the lines of:

// Somehow get this injected
__webpack_require__.$RefreshRuntime$ = require('react-refresh/runtime');

Do I need to somehow get the source of the module and evaluate it to string?

When I started writing the plugin I recall myself going down this path too, but was unable to properly make this work (without multiple evaluation passes/busting cache all the time).

@sokra
Copy link

sokra commented May 20, 2020

Try to place only __webpack_require__.$Refresh = {} in the webpack runtime and lazy initialize __webpack_require__.$Refresh.Runtime and set __webpack_require__.$Refresh.Sig in the module code.

It need to be an extra object as __webpack_require__ is different for each module with HMR and properties are copied.

You can't access modules in the runtime since they may not be loaded at this point, e.g. when using optimization.runtimeChunk.

@iiroj
Copy link
Contributor

iiroj commented May 26, 2020

I'm also having trouble with this. In my case, this plugin inserts itself into the polyfill bundle, breaking IE 11 because it's using URLSearchParams internally (which is not yet defined).

Would it be possible to expose something like @pmmmwh/react-refresh-webpack-plugin/runtime so that it could be manually added where ever?

@pmmmwh
Copy link
Owner

pmmmwh commented May 26, 2020

I'm also having trouble with this. In my case, this plugin inserts itself into the polyfill bundle, breaking IE 11 because it's using URLSearchParams internally (which is not yet defined).

Hmm ... The call to URLSearchParams should be called only after your code loads. Is it possible to create a minimal reproduction of what's happening?

Would it be possible to expose something like @pmmmwh/react-refresh-webpack-plugin/runtime so that it could be manually added where ever?

The difficult part is that there's both code to run BEFORE any user code, and AFTER all user code. The runtime, as of it's current state, is also quite coupled with the window global and some other injected dependencies (via the plugin). I will revisit this idea after I'm done with some core logic rewrite.

@iiroj
Copy link
Contributor

iiroj commented May 26, 2020

Thanks for the reply @pmmmwh. I'll get back to you if I can better isolate the issue (or if I manage to find it's because of something else).

@Lalitha-Iyer
Copy link

Lalitha-Iyer commented Jan 8, 2021

Is there a proper way to inject an external dependency to Webpack's require function (i.e. __webpack_require__)

I am facing a similar problem. I want the webpack runtime bundle to always include certain modules. This is turning out hard to do with multiple entry points ( each with their own runtime) as the custom module is extracted in a default common chunk by SplitChunk plugin. The use case I have is I am loading the bundle in a non-browser environment and I need to use custom modules for chunk fetching.
For my usecase it would be convenient if there was an option to exclude modules from being split into chunks.

@davecoates
Copy link

In case anyone is looking for a workaround for multiple entry points this worked for me:

    const BaseReactRefreshWebpackPlugin = require('@pmmmwh/react-refresh-webpack-plugin');
    class ReactRefreshWebpackPlugin extends BaseReactRefreshWebpackPlugin {
        apply(compiler) {
            super.apply(compiler);
            if (compiler.options.entry.djangoStyles) {
                // Remove all the hot module related packages from djangoStyles entry point.
                // Without this HMR breaks with error `Uncaught Error: Iframe has not been created yet.`
                // This came from react-error-overlay but this issue is likely related:
                // https://github.com/pmmmwh/react-refresh-webpack-plugin/issues/88
                // There's no option to disable it directly
                const excludeEntries = [
                    '@pmmmwh/react-refresh-webpack-plugin/client/ReactRefreshEntry.js',
                    '@alliance-software/webpack-dev-utils/client/hot-client-errors.js',
                    'webpack/hot/dev-server.js',
                ];
                compiler.options.entry.djangoStyles = compiler.options.entry.djangoStyles.filter(
                    path => excludeEntries.filter(p => path.endsWith(p)).length === 0
                );
            }
        }
    }

@pmmmwh
Copy link
Owner

pmmmwh commented Jun 1, 2021

If anyone is using Webpack 5 feel free to give the latest beta a try, it should inject everything as global instead on a specific entrypoint, effectively eliminating the issue.

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