Skip to content

Conversation

Sibirius
Copy link
Contributor

Summary

This PR upgrades webpack and related packages to version 5.

Description

The config changes needed are actually quite minimal:

Motivation and Context

We are using the contentful-management library in a React app and recently upgraded our build system to use webpack 5. After this upgrade we were seeing runtime errors relating to enums from this library not being defined.

We were able to narrow down the issue to the minimization step of webpack. With minimize: false everything is working fine. So I decided to give upgrading to webpack 5 in this repo a try and now it works for us.

I was able to successfully run build and test locally. Please let me know if there's anything else needed from my side.

Checklist (check all before merging)

  • Both unit and integration tests are passing
  • There are no breaking changes
  • Changes are reflected in the documentation

@Sibirius Sibirius requested a review from a team as a code owner July 21, 2023 12:31
@marcolink
Copy link
Member

Hi @Sibirius 👋
Thanks for your contributions 🙏
Would you be able to also remove babel-minify-webpack-plugin and its usage as it is deprecated and webpack handles minification itself already?

@Sibirius
Copy link
Contributor Author

@marcolink
I removed it now. Was thinking about that as well but wanted to keep the changes minimal as I wasn't sure if there was some specific reason for keeping this one around.
Happy to clean up even more if you have some suggestions.

@marcolink
Copy link
Member

@Sibirius this looks great 🎉

Do you know why it"s now also creating the LICENCE files in the dist folder? 🤔

BEFORE
Screenshot 2023-07-27 at 16 48 57

AFTER
Screenshot 2023-07-27 at 16 51 45

@Sibirius
Copy link
Contributor Author

@marcolink
This is the default behaviour of webpack 5 as far as I understand.
I could add the CleanWebpackPlugin or a custom plugin as suggested here: webpack/webpack#12506

@marcolink
Copy link
Member

marcolink commented Jul 28, 2023

@Sibirius

If you would be able to integrate CleanWebpackPlugin for that purpose, i'd be happy to merge this PR right away 🙏

@Sibirius
Copy link
Contributor Author

@marcolink
I didn't manage to get the CleanWebpackPlugin to work properly. It was always deleting other build artifacts as well even when I specifically configured it not to. Also there are talks in the issues that it might become deprecated.
So I went with a simple custom plugin. Hope that works for you.

@Sibirius
Copy link
Contributor Author

Sibirius commented Aug 2, 2023

@marcolink
Is there anything blocking the PR that I can help with?

@mayakarabula
Copy link
Contributor

@Sibirius
hey Sibirius, sorry for the delay, Marco went on vacation, and this PR got off our radar for a couple of days.
I will take a look at it now.

@mayakarabula
Copy link
Contributor

mayakarabula commented Aug 4, 2023

@Sibirius
hey again, I have tried to use the package built by webpack 5, but it looks like the export pattern is different, it exports an object contentfulManagement instead of functions directly as before, so for example:

const contentful = require('./dist/contentful-management.node')
console.log({ contentful })

results in

{
  contentful: {
    contentfulManagement: Object [Module] {
      RestAdapter: [Getter],
      ScheduledActionReferenceFilters: [Getter],
...

but it should have root-level exports for RestAdapter, createClient, etc, because otherwise, that would be a breaking change.
I am not super familiar with webpack configurations but I was able to get root-level exports when modifying line 50 in webpack.config.js - commenting the library parameter

    // library: 'contentfulManagement',
{
  contentful: Object [Module] {
    RestAdapter: [Getter],
    ScheduledActionReferenceFilters: [Getter],
...

If this sounds reasonable (to remove library param) I would suggest making this change in PR or otherwise getting the root-level exports

@Sibirius
Copy link
Contributor Author

Hey @mayakarabula 👋
Sorry, for the delay on my side this time, as I was on vacation last week.
The change definitely seems reasonable to me 👍

Should I just close this one, since it's now continued in #1921 ?

@mayakarabula
Copy link
Contributor

@Sibirius
No worries! feel free to continue with this PR, I have created the other one to do the change myself but I think it's better to use this PR as it has all the conversation history

@Sibirius
Copy link
Contributor Author

@mayakarabula
Great! I've merged your changes in here. Should be good to go then.

@mayakarabula
Copy link
Contributor

thank you! I will merge it

@mayakarabula mayakarabula merged commit 48fdbe1 into contentful:master Aug 21, 2023
kdamball pushed a commit that referenced this pull request Aug 22, 2023
* chore: upgrade webpack to version 5

* fix: update webpack config for new version

* chore: remove babel-minify-webpack-plugin

* chore: remove license.txt files

* remove library parameter

---------

Co-authored-by: Marco Link <[email protected]>
Co-authored-by: Maya Karabula-Stysiak <[email protected]>
@contentful-automation
Copy link
Contributor

🎉 This PR is included in version 10.40.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants