-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
upgrade to webpack 4 #4112
Conversation
This pull request introduces 1 alert when merging 877c763 into bc3987d - view on LGTM.com new alerts:
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. |
There was a problem hiding this 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.
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 |
I have checked it again and its working. The |
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:
|
This pull request introduces 1 alert when merging 93e201f into 099a723 - view on LGTM.com new alerts:
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 |
So I'm not sure I'm a fan of the copy/paste of the I think the 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 |
…pgrade # Conflicts: # package-lock.json # package.json
@sumit116 feel free to look at the latest updates I made. It's back to the 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. |
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:
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 |
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. |
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. |
Type of change
Description of change
Upgrate to webpack 4