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

upgrade to webpack 4 #4112

Closed
wants to merge 16 commits into from
Closed

upgrade to webpack 4 #4112

wants to merge 16 commits into from

Conversation

sumit116
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Upgrate to webpack 4

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 877c763 into bc3987d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Warning - Automated code review for prebid/Prebid.js will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@sumit116 sumit116 changed the title Rad 2815 webpack4 upgrade upgrade to webpack 4 Aug 23, 2019
@sumit116
Copy link
Contributor Author

This pull request introduces 1 alert when merging 877c763 into bc3987d - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Warning - Automated code review for prebid/Prebid.js will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.

Comment posted by LGTM.com

Removed unused var.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch still has not addressed the issue with the prebid-core.js chunk never executing. The test page will not work with this bundle.

I think it may be a bug in the webpack splitChunks feature. I'm trying to recreate in an isolated environment.

@snapwich
Copy link
Collaborator

I've been able to reproduce the bug in isolation here: https://github.com/snapwich/webpack4-chunk-bug

I also found what I think is a duplicate of the same issue we're experiencing here: webpack/webpack#7230

There doesn't appear to be a fix yet :(

@sumit116
Copy link
Contributor Author

I've been able to reproduce the bug in isolation here: https://github.com/snapwich/webpack4-chunk-bug

I also found what I think is a duplicate of the same issue we're experiencing here: webpack/webpack#7230

There doesn't appear to be a fix yet :(

@snapwich , I found that you are using enforce: true in splitchunks cacheGroups which is not allowing the common chunk (entry) to execute. Removing it worked for me. I am not sure why this property is creating this issue, but I checked the docs and found its only needed when you want to enforce deviation from the default values of minSize or minChunks, etc. I don't think we require to do that and even if we want to we can alternatively pass their respective values in the chunk object under cacheGroups .

@sumit116
Copy link
Contributor Author

This branch still has not addressed the issue with the prebid-core.js chunk never executing. The test page will not work with this bundle.

I think it may be a bug in the webpack splitChunks feature. I'm trying to recreate in an isolated environment.

I have checked it again and its working. The prebid-core chunk is getting executed and I am able to run the hello_world example successfully. Can you check if you have any local changes like enfore: true or can you reinstall your packages?

@snapwich
Copy link
Collaborator

So it still appears broken to me, and I'm pretty sure it's related to that webpack issue I linked to.

At this point I think we have a few options:

  1. We keep spending more time on this (I've spent hours and it doesn't look like it's going anywhere productive).
  2. We make more noise on the webpack issue and hope it gets fixed. Since the issue is over a year old at this point and proposals have been suggested that were not merged, I don't have much faith this will help.
  3. We attempt one of the hacky solutions suggested in the thread or fork webpack (I think that's what the airbnb peeps ended up doing?).
  4. We try to re-architect our solution without using the splitChunks plugin. (someone suggested maybe the DLLPlugin could do what we want)
  5. We don't upgrade.

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 93e201f into 099a723 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

Warning - Automated code review for prebid/Prebid.js will be disabled on October 1, 2019. You can avoid this by installing the LGTM.com GitHub App. Read about the benefits of migrating to GitHub Apps in the blog.


Comment posted by LGTM.com

@snapwich
Copy link
Collaborator

snapwich commented Sep 4, 2019

So I'm not sure I'm a fan of the copy/paste of the splitChunks plugin. It cuts us off from the upstream and puts us in a worse spot for future upgrades, which is not a good place to be in with webpack 5 just around the corner.

I think the DLLPlugin does what we want, unfortunately after some experimentation yesterday it seems that it doesn't work with webpack-stream: shama/webpack-stream#149

I'm seeing if there's a way around that issue and investigating other solutions at the moment.

@sumit116
Copy link
Contributor Author

sumit116 commented Sep 4, 2019

So I'm not sure I'm a fan of the copy/paste of the splitChunks plugin. It cuts us off from the upstream and puts us in a worse spot for future upgrades, which is not a good place to be in with webpack 5 just around the corner.

I think the DLLPlugin does what we want, unfortunately after some experimentation yesterday it seems that it doesn't work with webpack-stream: shama/webpack-stream#149

I'm seeing if there's a way around that issue and investigating other solutions at the moment.

Thanks for your feedback and looking into the issues for this upgradation. The copy of SplitChunksPlugin was added as the hacky solution mentioned in #3 of #4112 (comment). Also, as discussed, using DLLPlugin would result in making some changes in the build process, which we would need to take care of too.

@snapwich
Copy link
Collaborator

@sumit116 feel free to look at the latest updates I made. It's back to the splitChunks plugin, however I generate both a prebid-core.js and prebid-shared.js, the former being the entry point that runs (but only has the webpack runtime) and the later that has all the shared code. Then I just concat them together back into prebid-core.js. I think this should work?

Also, not sure why but the bundle is a bit bigger with this webpack upgrade. The webpack runtime is about 4kb bigger but that doesn't account for all of it.

@snapwich snapwich removed the on hold label Sep 17, 2019
@sumit116
Copy link
Contributor Author

sumit116 commented Sep 17, 2019

I tried with a few things to decrease the Prebid bundle size but apart from creating a vendors (node_modules) chunk, could not get any significant decrease in size. We did not create a vendors chunk in webpack 3 since there is no reason to have it, so we do not add it in webpack 4 too. I compared the difference in size of current bundle and bundle after webpack4 upgrade:

with webpack 3 - 1085 kb (after 7zip - 229kb).
with webpack 4 - 1200kb (after 7zip - 233kb).

Ideally we should have the same size but it seems like webpack4 increased the size keeping same configurations. Also, as per discussion with @snapwich, we can ignore the size issue as it shouldn't be a big issue.

Besides this, I have tested the build and serve commands and the bundle with hello_world example and found things working fine.

cc: @mkendall07

@sumit116 sumit116 requested review from mkendall07 and jaiminpanchal27 and removed request for jaiminpanchal27 and snapwich September 17, 2019 16:07
@sumit116 sumit116 assigned jaiminpanchal27 and unassigned idettman Sep 19, 2019
@stale
Copy link

stale bot commented Oct 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 3, 2019
@snapwich snapwich added pinned won't be closed by stalebot and removed stale labels Oct 7, 2019
@mkendall07
Copy link
Member

since there is substantial size increase here and no tangible benefits I think we should defer this can be fixed properly (or Webpack 5 is released).

Feel free to reopen if there is a strong reason to proceed here.

@mkendall07 mkendall07 closed this Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned won't be closed by stalebot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants