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

Further modularizing .dev.dll #1468

Closed
nessup opened this issue Mar 13, 2018 · 2 comments
Closed

Further modularizing .dev.dll #1468

nessup opened this issue Mar 13, 2018 · 2 comments

Comments

@nessup
Copy link

nessup commented Mar 13, 2018

Since the module property of webpack.config.renderer.dev.dll contains the same thing as the module property in webpack.config.renderer.dev, I'm wondering if there was there a reason for the repetition. If not, maybe we could rewrite webpack.config.renderer.dev.js and webpack.config.renderer.dev.dll.js sorta like this:

// In webpack.config.renderer.dev.dll...
export default merge.smart(baseConfig, {
  // ...
  module: require('./webpack.config.renderer.dev').module
  // ...

// In webpack.config.renderer.dev...
if (module.parent.filename !== 'webpack.config.renderer.dev.dll.js' && !(fs.existsSync(dll) && fs.existsSync(manifest))) {

That's the basic idea. Might look weird or have other unintended side effects, but curious if something like this makes sense or can be done better? Happy to PR with a working version

@amilajack
Copy link
Member

Good suggestion. The configs actually used to be more modular. But here's the issue:
If the dll isn't built, webpack.config.renderer.dev.js will run yarn build-dll, which build's the dll. But the webpack.config.renderer.dev.dll.js imports the webpack.config.renderer.dev.js. So here's the cycle:

build-dll => import dev dll => imports dev => build-dll => import dev dll => imports dev
...and so on

But there should be a workaround for this. Simplifying configs when possible is definitely the direction the boilerplate should be heading in

@nessup
Copy link
Author

nessup commented Mar 16, 2018

I'll send in a PR with one possible solution soon. Thank you!

nessup added a commit to nessup/electron-react-boilerplate that referenced this issue Apr 5, 2018
Fixes electron-react-boilerplate#1468

To avoid a circular dependency, this relies on a check against the parent module's filename for `webpack.config.renderer.dev.dll.js`. It's expected that developers who rename config files will hopefully grep the codebase for this filename before changing it.

This change also includes this check again when configuring `DllReferencePlugin`, because it is no longer guaranteed that the DLL manifest file exists by the time the renderer config specifies plugins.

I played around with the idea of creating a `webpack-merge` strategy that plucks the value of `module` from the dev webpack config, but it wouldn't help much because, in `webpack.config.renderer.dev.js`, the `DllReferencePlugin` would still try to require the manifest (which would not exist).
amilajack pushed a commit that referenced this issue May 2, 2018
)

* Remove jsdom dep (#1411)

* Remove dynamic import dep (#1408)

* Add .sass files support (#1412)

* Update webpack.config.renderer.dev.dll.js

* update webpack to support sass files

* update webpack to support sass files

* Misc code style changes to menu.js

* Use module property from dev webpack config in DLL webpack config

Fixes #1468

To avoid a circular dependency, this relies on a check against the parent module's filename for `webpack.config.renderer.dev.dll.js`. It's expected that developers who rename config files will hopefully grep the codebase for this filename before changing it.

This change also includes this check again when configuring `DllReferencePlugin`, because it is no longer guaranteed that the DLL manifest file exists by the time the renderer config specifies plugins.

I played around with the idea of creating a `webpack-merge` strategy that plucks the value of `module` from the dev webpack config, but it wouldn't help much because, in `webpack.config.renderer.dev.js`, the `DllReferencePlugin` would still try to require the manifest (which would not exist).

* Use includes() rather than indexOf()
vikr01 pushed a commit to vikr01/electron-react-boilerplate that referenced this issue Jun 27, 2018
* Misc code style changes to menu.js

* v0.13.3

* More consistent node path

* Allowed node_modules to be checked by flow

* add prettier to format js files (electron-react-boilerplate#1418)

* Remove jsdom dep (electron-react-boilerplate#1411)

* Remove dynamic import dep (electron-react-boilerplate#1408)

* Add .sass files support (electron-react-boilerplate#1412)

* Update webpack.config.renderer.dev.dll.js

* update webpack to support sass files

* update webpack to support sass files

* Misc code style changes to menu.js

* chore: add perttier husky lint-stage eslint-config-prettier and add scripts

* refactor: use prettier to format code

* fix: fix lint error and add eslint prettier config

* fix: replace registry url from registry.npmjs.org to registry.yarnpkg.com for new add package

* docs: update changelog

* chore: add format-fix script and make format script just to check which files need to format

* format: use prettier to format webpack files

* docs: update change log - add format-fix script

* feat: add prettier in `lint-fix` script

* Removed unnecessary deps

* Updated deps

* Upgraded to webpack 4

* Run prettier even if eslint fails

* createBrowserHistory to createHashHistory for prod (electron-react-boilerplate#1184)

* Filter deps without entrypoint from dll

* Bumped deps

* Temporary hack to get flow working with webpack-cli

* Use module property from dev webpack config in DLL webpack config (electron-react-boilerplate#1497)

* Remove jsdom dep (electron-react-boilerplate#1411)

* Remove dynamic import dep (electron-react-boilerplate#1408)

* Add .sass files support (electron-react-boilerplate#1412)

* Update webpack.config.renderer.dev.dll.js

* update webpack to support sass files

* update webpack to support sass files

* Misc code style changes to menu.js

* Use module property from dev webpack config in DLL webpack config

Fixes electron-react-boilerplate#1468

To avoid a circular dependency, this relies on a check against the parent module's filename for `webpack.config.renderer.dev.dll.js`. It's expected that developers who rename config files will hopefully grep the codebase for this filename before changing it.

This change also includes this check again when configuring `DllReferencePlugin`, because it is no longer guaranteed that the DLL manifest file exists by the time the renderer config specifies plugins.

I played around with the idea of creating a `webpack-merge` strategy that plucks the value of `module` from the dev webpack config, but it wouldn't help much because, in `webpack.config.renderer.dev.js`, the `DllReferencePlugin` would still try to require the manifest (which would not exist).

* Use includes() rather than indexOf()

* Updated all deps to latest semver

* Bumped deps

* Bumped all deps to latest semver

* Update changelog

* Increased delay for e2e counter test

* Updated lock file

* Bumped ci node versions

* Reverted version change in CHANGELOG [ci skip]
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

No branches or pull requests

2 participants