search

electron-userland / electron-webpack

Scripts and configurations to compile Electron applications using webpack
https://webpack.electron.build/
905 stars 171 forks source link

Built release app not working for me with v2.6.2, worked with v2.6.1 #275

Open ronen opened 5 years ago

ronen commented 5 years ago

Hi. Sorry, this isn't going to be a very useful bug report, since I don't have a small reproducible case.

But as an FYI/heads-up, when I update electron-webpack from v2.6.1 to v2.6.2 (and make no other changes to my setup), my release app stops working. It has something to do with react-redux. Here are the details FWIW:

Of course this might be a react-redux issue rather than an electron-webpack issue. Or some weird problem in my own code.

But since the only change in my setup is the electron-webpack patch version I figured I'd at least start by mentioning it here, in case this description sparks a thought for somebody else. Thanks for listening! :)

vgribdev commented 5 years ago

It works after the revert of this commit d2433f6

zenios commented 5 years ago

Same happened to me

bompi88 commented 5 years ago

I experience the same thing. Adding react-redux to the whiteListedModules, fixes it in my case:

"electronWebpack": {
  "whiteListedModules": [
     "react-redux"
  ]
},
ronen commented 5 years ago

@bompi88's solution fixes it for me too (thanks!).

I don't know if this is a bug or should be expected behavior (in which case IMHO it should somehow be documented, and arguably should be marked as a breaking change rather than patch update) -- so I'm going to leave this issue open, to let the electron-userland team comment on it and close it if/as they see fit.

fabiospampinato commented 5 years ago

I'm the person who introduced the troublesome commit (https://github.com/electron-userland/electron-webpack/commit/d2433f6) and I just stumbled upon this error as well.

The problem is that since react and react-dom are being whitelisted from the external dependencies list they are being shipped along with the app bundle, so our apps' code load those, while other libraries located in node_modules I think will still load the version of react and react-dom found under node_modules.

I'm not sure is this error surfaces also in production builds, I can't check at the moment.

Anyway the deeper issue seems that having external dependencies basically at all breaks at least the development mode.

@loopmode This seems a pretty important issue, how do you think we should proceed? I think maybe as a quick workaround the troublesome commit can be reverted, and maybe in order to fix this properly external dependencies should be ignored completely while in development mode?

zenios commented 5 years ago

+1

On Fri, 31 May 2019, 22:56 Fabio Spampinato, notifications@github.com wrote:

I'm the person who introduced the troublesome commit (d2433f6 https://github.com/electron-userland/electron-webpack/commit/d2433f6) and I just stumbled upon this error as well.

The problem is that since react and react-dom are being whitelisted from the external dependencies list they are being shipped along with the app bundle, so our apps' code load those, while other libraries located in node_modules I think will still load the version of react and react-dom found under node_modules.

I'm not sure is this error surfaces also in production builds, I can't check at the moment.

Anyway the deeper issue seems that having external dependencies basically at all breaks at least the development mode.

@loopmode https://github.com/loopmode This seems a pretty important issue, how do you think we should proceed? I think maybe as quick workaround the troublesome commit can be reverted, and maybe in order to fix this properly external dependencies should be ignored completely while in development mode?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/electron-userland/electron-webpack/issues/275?email_source=notifications&email_token=AAAV3YODPKUXZXOLBOJFNZ3PYF7FZA5CNFSM4G4F4HG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWWHFTY#issuecomment-497840847, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAV3YOD27NUO2WADO34JZTPYF7FZANCNFSM4G4F4HGQ .

fabiospampinato commented 5 years ago

Following on my previous comment: not loading react and react-dom as external dependencies cause the development version of those libraries to be loaded in production mode, which is not good. Perhaps this is a separate issue though.

loopmode commented 5 years ago

@fabiospampinato did you try with manually setting NODE_ENV=production?

loopmode commented 5 years ago

Regarding your previous comment and question: I think one of the biggest problems electron-webpack has is that it's trying too much to make things really easy for users, and implicitly adding react and react-dom to whitelisted modules is one symptom of that basic problem. I think it's better to put the effort into documentation and more explicit mechanisms. Please don't take this as criticism of your commit back in the day - I myself have done worse :) like automatically adding all devDependencies starting with @babel or babel- to the config, then telling users to put their Babel stuff into regular instead of devDependencies so it doesn't get added right away..

So, I'm not sure how to proceed, but my feeling is that reverting the commit is indeed a step in the right direction. And making it really clear that one needs to manually add the libs to whitelisted modules - if that is a solution.

Not sure at all about the development mode, I really haven't gotten much into webpack externals and the whitelisting thing - so I just don't have a qualified opinion here, and currently no time to catch up. But I thought I'd better respond here now nonetheless before I forget it for weeks or months :)

Now that react hooks are available, it's really important to make sure there is exactly one react and react-dom loaded, and in proper production mode for production builds, or electron-webpack will become useless for react projects.

fabiospampinato commented 5 years ago

did you try with manually setting NODE_ENV=production?

@loopmode yeah but that's tricky.

First of all webpack is configured to replace process.env.NODE_ENV with the environment string, in order to enable some dead code elimination optimizations. This works only for code in the bundle though, that's why the prod version of React is loaded only when it's excluded from the external dependencies.

Passing NODE_ENV=production in my builder script should be useless, because that env variable is retrieved at runtime by react.

I could pass NODE_ENV=production to the actual script that spawns up Electron, but how? I'm not really sure how to do that.

The thing that worked for me, and that's a pretty ugly solution, was to store the value of process.env.NODE_ENV in a custom module, let's call it Environment, and then at runtime, before React is loaded, setting process.env[`NODE_ENV`] = Environment.name. Notice the super ugly property access, otherwise webpack would have replaced that with a string and the assignment would have thrown an exception.

Bottom line is: most dependencies should actually be loaded as non-external dependencies (some really can't though, as webpack isn't able to bundle all kinds of things), because that ensures that only the necessary code is loaded. For instance by loading React as an external dependency both the dev and prod versions of it will be included in my app's asar archive. And the environment should be set when actually running Electron as well.

and implicitly adding react and react-dom to whitelisted modules is one symptom of that basic problem

Yeah totally.

And making it really clear that one needs to manually add the libs to whitelisted modules - if that is a solution.

I think it would be a solution if external modules were only considered during production. Currently doing that may cause conflicting dependencies to be loaded (i.e. multiple version of react).

Now that react hooks are available, it's really important to make sure there is exactly one react and react-dom loaded, and in proper production mode for production builds, or electron-webpack will become useless for react projects.

Exactly. Another option, and maybe the best one short-term wise, could be to add react and react-dom to the whitelist only on production. That's totally too hacky to be a reliable solution that works across different stacks, but in the short term ensures that both react apps keep working and the production version of react (which is ~2-8x times faster) is loaded when bundling the app.

funwithtriangles commented 4 years ago

Has any further progress been made on this or any updates to the documentation on how to deal with it? The easiest thing for me right now is to stay with version 2.6.1 but I'm sure I'll need to upgrade eventually.

obra commented 4 years ago

I'm also running into this. The change in 2.6.2 caused a really, really weird behaviour where serialport broke when used in react...but only after a few interactions.

@loopmode, would it be reasonable to revert the commit (https://github.com/electron-userland/electron-webpack/commit/d2433f6) that caused this for the next release as @fabiospampinato suggested last summer?

loopmode commented 4 years ago

I'm not sure I am qualified to answer that, I've never done that and it would be a question for @develar

However, I'd like to point out that if you're using react, you can simply add react and react-dom to the whitelisted modules yourself. (Maybe that's not clear to some users coming here..)

alexweber commented 4 years ago

FWIW, adding React and React Dom to whitelisted modules didn't do anything for me, however adding them to webpack externals makes things work smoother...

loopmode commented 4 years ago

Hm. I don't quite understand that one. The default in electron-webpack is that all dependencies are added to webpack externals, unless whitelisted. Did you install react and react-dom as devDependency maybe?

alexweber commented 4 years ago

nevermind i was being dumb :)

funwithtriangles commented 4 years ago

For anyone else who was having trouble with the white listed modules fix, you really need to add every package that is related to React, or might have React as a dependency. I added every dependency of my project to the whitelist, then removed packages until I got the error again. Just to prove how ugly this can be, here is what I ended up with:

"whiteListedModules": [
      "connected-react-router",
      "create-react-class",
      "react",
      "react-addons-perf",
      "react-create-class",
      "react-dom",
      "react-hot-loader",
      "react-input-autosize",
      "react-popper",
      "react-redux",
      "react-router",
      "react-router-dom",
      "react-sortable-hoc",
      "recompose",
      "redux-batched-subscribe",
      "redux-debounce",
      "redux-form",
      "redux-ignore",
      "redux-saga",
      "styled-components"
 ],
loopmode commented 4 years ago

Actually, I'd recommend going about it the other way round: simply replace externals by an empty array and add packages to it that really need it. For most user's that will be: no packages at all. You only need to add certain dependencies you use in the main process to webpack externals, but typically none used in the renderer process (e.g. react and friends)

// webpack.renderer.config.js
module.exports = function(config) {
    // ...
    config.externals = [];
    return config;
}
loopmode commented 4 years ago

Reminder about how the mechanism works: electron-webpack puts all dependencies to webpack externals automatically, unless they're whitelisted. However, all of that happens before the config object passes through your custom config function, so you can basically undo the feature as described.