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

Use chunk.forEachModules instead of deprecated chunk.modules #328

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
2 participants
@janicduplessis
Copy link
Contributor

janicduplessis commented Feb 26, 2018

What did you implement:

Webpack 4 removes the deprecated chunk.modules property. This uses the forEachModule instead. See https://github.com/webpack/webpack/blob/webpack-3/lib/Chunk.js#L473.

forEachModule exists since webpack v3 so this means webpack v2 is no longer supported with this change. Not sure how much we care about webpack 2 support. If we want we could check if forEachModule exists and fallback to .modules.

How did you implement it:

Trivial, most changes were to update tests.

How can we verify it:

  • Tested in a project using webpack 4, before this change externals were broken.
  • Run tests

Todos:

  • Write tests (N/A)
  • Write documentation (N/A)
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: YES (if webpack 2 is still supported)

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Feb 26, 2018

Hi @janicduplessis, thanks for the PR. It indeed makes sense to switch to the no-deprecated chunk module enumerator.

Regarding webpack 2 support, I think there are a lot of serverless projects out there that make use of it. Either we schedule this change for serverless-webpack@5.0.0 and drop webpack 2 support completely or we implement a fallback mechanism.

However, I'd prefer a new major version, because as the old functionality is deprecated, it is much cleaner to continue with only the correct implementation and drop webpack 2 support.

@janicduplessis

This comment has been minimized.

Copy link
Contributor Author

janicduplessis commented Feb 26, 2018

@HyperBrain I also think scheduling this for 5.0.0 is better. It would add support for webpack@4 and drop webpack@2.

@HyperBrain HyperBrain added this to the 5.0.0 milestone Feb 26, 2018

@HyperBrain HyperBrain changed the base branch from master to v5 Feb 28, 2018

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Feb 28, 2018

I retargeted this PR to the new v5 branch. According to #331 this is urgently needed to support webpack v4, so we'll prepare the v5 branch to gather everything needed to support it.

@janicduplessis Did you try already, if this PR enabled webpack 4 support?

@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Feb 28, 2018

Tested in a project using webpack 4, before this change externals were broken.

Sorry 😄 . Did not read that.
The PR looks good to me, so I'll merge it into the v5 branch now.

@HyperBrain HyperBrain merged commit e6e544e into serverless-heaven:v5 Mar 1, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.41%
Details
@HyperBrain

This comment has been minimized.

Copy link
Member

HyperBrain commented Mar 8, 2018

Released with 5.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment