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

Outdated javascript dependencies #6343

Open
1 task done
niklasnatter opened this issue Nov 2, 2021 · 21 comments
Open
1 task done

Outdated javascript dependencies #6343

niklasnatter opened this issue Nov 2, 2021 · 21 comments
Labels
help wanted Technical Debt Impacts code quality, no or just small impact on end developers and users

Comments

@niklasnatter
Copy link
Contributor

niklasnatter commented Nov 2, 2021

Unfortunately, we cannot update some javascript packages because of various reasons. This issue tracks which packages are affected and why we need to use an outdated package version:

Package Used Version Available Version Reason
mobx 4 6 contains breaking changes for project code, comeback of decorators (mobxjs/mobx#3638) will make this easier
jest / babel-jest 26 29 tests are failing because of comparison changes: #6361
react 17 18 our testing library enzyme is not compatible with react@18: https://dev.to/wojtekmaj/enzyme-is-dead-now-what-ekl #6689
@testing-library/react 12 - we need to update react first
@testing-library/jest 5 - we need to update jest first
flow-bin 0.142 0.194 requires big upgrade effort because flow-bin@0.143 dropped classic mode. it might be better to switch to typescript than making this effort
mobx-react 5 7
react-leaflet 3 4 depends on react@18: #6689

Webpack 5:

@niklasnatter niklasnatter added the Technical Debt Impacts code quality, no or just small impact on end developers and users label Nov 2, 2021
@danrot
Copy link
Contributor

danrot commented Dec 22, 2021

I'd be interested to update webpack 5, so that we could maybe later fix sulu/skeleton#88 by introducing module federation.

The good news is that ckeditor seems to have fixed their issue with Webpack 5.

But the linked commit from ckeditor has not been released yet. And when thinking about it, since multiple ckeditor packages are in use, it might be hard to get all of that working when not being able to rely on releases, so it might be worthwhile waiting for the release to start working on this.

While this is nothing that you can have an influence on, there is a second question that I feel has to be asked: Would you consider upgrading to webpack 5 to be a breaking change? I think strictly speaking it is, since Sulu allows to extend the admin interface by adjusting its webpack config, and upgrading to webpack 5 might cause many custom admin build to fail. Is that something that should be done for 3.0 only?

EDIT:
Seems like they are currently planning to release the new version at the 19th of January (ckeditor/ckeditor5#10987).

@niklasnatter
Copy link
Contributor Author

I'd be interested to update webpack 5, so that we could maybe later fix sulu/skeleton#88 by introducing module federation.

The good news is that ckeditor seems to have fixed their issue with Webpack 5.

Thats great news and would allow us to update most of the packages listed above 🥳

While this is nothing that you can have an influence on, there is a second question that I feel has to be asked: Would you consider upgrading to webpack 5 to be a breaking change? I think strictly speaking it is, since Sulu allows to extend the admin interface by adjusting its webpack config, and upgrading to webpack 5 might cause many custom admin build to fail. Is that something that should be done for 3.0 only?

Yes, strictly speaking it would be a breaking change. But I think we decided to tolerate breaking changes in the frontend part in minor releases, so it would be okay for me to do this in a possible 2.5 version.

@alexander-schranz
Copy link
Member

With the already exist dll builds we maybe get rid of the whole CKEditor webpack specific configurations already:

https://ckeditor.com/docs/ckeditor5/31.1.0/builds/guides/development/dll-builds.html#using-a-dll-build

https://jsfiddle.net/vtg19hry/

That would be in my case the best for ckeditor as I really don't want that we need to maintain ckeditor specific webpack configs.

@alexander-schranz
Copy link
Member

Looks like the next ckeditor version 30.0 will be using webpack 5 and postcss 8: ckeditor/ckeditor5-dev#764

@alexander-schranz
Copy link
Member

React 18 was released, I added it to the list, I think there should be not a lot required to upgrade to it: https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html

@alexander-schranz
Copy link
Member

In todays CMS Garden meeting @MrTango presented the Module Federation implementation of Plone 6. So we can find inspiration there how to configure webpack as it requires special configuration that all dependencies are provided as own module federation and they did work together with some webpack module federation dev to get it work well together. So when we tackle the module federation issue we can also contact @MrTango to get some insights from Plone and their implementation.

@alexander-schranz
Copy link
Member

I did have a look today at webpack 5 update. But as it is a breaking change as webpack does not longer polyfill node libraries and make so some dependency crash I decided that we will move the webpack update to Sulu 3.0. Currenty state can be seen here: #6690. While there are workarounds about this, it would make still projects crash depending on such libraries and so we should do this only in a new major and in that case cleanup our dependencies not depending on any node libraries any more.

Still we will update postcss, ckeditor and react in version 2.5 to latest versions.

@alexander-schranz
Copy link
Member

Currently enzyme is one of the blocking libraries and we are currently working on the migration to react-testing-library: Issue #6799

@alexander-schranz
Copy link
Member

alexander-schranz commented Nov 29, 2022

I added a comment in the issue description about our now hardcoded browserlist because of the webpack 4 terser parser, which can be removed when updating to webpack 5.

@alexander-schranz
Copy link
Member

alexander-schranz commented Dec 1, 2022

Webpack 5 merged 🎉

Removed it from the list.

@niklasnatter
Copy link
Contributor Author

Should we remove the hardcoded browserlist/babel configuration now that Webpack 5 was merged?

@alexander-schranz
Copy link
Member

@niklasnatter sure we could also try to update the other webpack loaders.

@alexander-schranz
Copy link
Member

alexander-schranz commented Dec 5, 2022

Current list from npm outdated of sulu/sulu (this seems not list dependencies of depenndencies (e.g. sulu-admin-bundle ckeditor packages)):

@ckeditor/ckeditor5-dev-utils        30.5.0   30.5.0  31.1.13  sulu-sulu
@ckeditor/ckeditor5-theme-lark       34.2.0   34.2.0   35.3.2  sulu-sulu
@testing-library/react               12.1.5   12.1.5   13.4.0  sulu-sulu
@wojtekmaj/enzyme-adapter-react-17    0.6.7    0.6.7    0.8.0  sulu-sulu
babel-jest                           26.6.3   26.6.3   29.3.1  sulu-sulu
babel-loader                          8.3.0    8.3.0    9.1.0  sulu-sulu
css-loader                            5.2.7    5.2.7    6.7.2  sulu-sulu
dependency-cruiser                  11.18.0  11.18.0   12.1.0  sulu-sulu
flow-bin                            0.142.0  0.142.0  0.194.0  sulu-sulu
jest                                 26.6.3   26.6.3   29.3.1  sulu-sulu
mobx                                 4.15.7   4.15.7    6.7.0  sulu-sulu
mobx-react                            5.4.4    5.4.4    7.6.0  sulu-sulu
postcss-import                       14.1.0   14.1.0   15.0.1  sulu-sulu
postcss-loader                        4.3.0    4.3.0    7.0.2  sulu-sulu
postcss-nested                        5.0.6    5.0.6    6.0.0  sulu-sulu
postcss-simple-vars                   6.0.3    6.0.3    7.0.1  sulu-sulu
react                                17.0.2   17.0.2   18.2.0  sulu-sulu
react-dom                            17.0.2   17.0.2   18.2.0  sulu-sulu
react-styleguidist                   11.2.0   11.2.0   13.0.0  sulu-sulu
react-test-renderer                  17.0.2   17.0.2   18.2.0  sulu-sulu
stylelint-config-standard-scss        5.0.0    5.0.0    6.1.0  sulu-sulu

The first step will mostly be update ckeditor to latest version and check then what else we can update.

@alexander-schranz
Copy link
Member

CKEditor update currently blocked by a strange error in: #6926

Did create an upstream issue in ckeditor maybe they know what change is providing: ckeditor/ckeditor5#12971

@alexander-schranz
Copy link
Member

alexander-schranz commented Dec 6, 2022

The css-loader update will be little bit more work and I'm not sure if we can tackle that before its done by ckeditor itself.

The css-loader update deprecates file-loader, raw-loader, url-loader, ... and requires us to replace it with the new Asset modules: https://webpack.js.org/guides/asset-modules/

I tried to migrate but did fail with the CKEditor here: ckeditor/ckeditor5#12979. Tried to update in #6930

@niklasnatter
Copy link
Contributor Author

niklasnatter commented Dec 9, 2022

I wanted to add a few thoughts on the remaining outdated packages and and how to tackle them. I think there are 3 big set of packages we need to tackle before the next major version. Some of them will affect projects with custom Javascript.

Unfortunately, I do not see a way around this and I dont think we should stay on the outdated versions because this will cause more pain on our side in the future. We already have some problems that are caused by outdated packages (we cannot update jest because our mobx version is not compatible with the matcherst of the new versions) and this class of problem will only grow bigger. This holds especially true for "big" dependencies like react.

flow-bin

I am convinced that the best option for moving forward here is to move away from flow and use TypeScript in the next major version. Fortunately, there are multiple companies (most noteworthy Stripe) that completed this switch previously and blogged about it:

I think the best option here is to fork the codemod used by Stripe, adjust it to our needs and do the migration in one big step. I dont think a gradual migration will work unfortunately:

To prevent complicated merge conflicts everytime we need to change the Javascript of the old major version, we could think about stripping all flow types from our Javascript code once the Typescript migration is completed.

This change will affect projects that use flow in their Javascript code and depend on the type in our codebase. Fortunately, we never published any tutorials or examples for this and I dont think there are many projects out there doing this.

react and enzyme

We need to update our react package eventually. We need to migrate all test cases to the react testing library (see #6799). We already started with this migration and things went pretty smoothly, but it is still a massive amount of work that needs to be done. Unfortunately, I do not think there is a way around this and we need to bite the bullet.

IMO, the best option for tackling this would be getting the whole team together for a few days and work on migrating the test cases exclusively. This would be nice because we could share the pain of working on this. Also, there will be pitfalls and this would ensure efficient knowledge transfer between the team. I already collected a few helpful resources about the update in the initial pull request: #6711

The good thing about this part is that the upgrade should not affect any projects and we can do it in a minor/patch version.

mobx and jest

mobx had to move away from decorators with version 6. Because of this upgrading our code means that we need to adjust most of our components and also our stores. Hopefully, once we updated our mobx version we can just update our jest version without any additional problems. The following resources are helpful to understand the type of changes we need to make:

Specifically, most of the changes should basically be using the makeObservable function in our stores/components to make the properties of the store observable again:

constructor() {
    makeObservable(this)
}

This should be relatively straightforward and it might be possible to automate a lot of it via the mobx-undecorate codemod. I am not sure if the codemod is compatible with flow, but it might be possible to fork and adjust it. So I am cautiously optimistic that this migration will be a lot easier than the two above. Still, we do have a lot of components and the migration will take some time and require some testing.

After finishing the migration, I think it would make sense to add the eslint-plugin-mobx to our eslint configuration.

As there is only a single mobx package per project (it is a peerDependency of all bundles), this change will affect all projects that used mobx in their custom code. If a project contains a store/component implemented with mobx, the file needs to be adjusted similar to what we need to do inside of Sulu. Therefore, I think we should do this only in a new major version and document it properly in our README.md.

@alexander-schranz
Copy link
Member

Should we remove the hardcoded browserlist/babel configuration now that Webpack 5 was merged?

The required changes only for webpack 4 terser parser were removed, this includes some @babel plugins

PRs: sulu/skeleton#211 and sulu/skeleton#211.

@alexander-schranz
Copy link
Member

alexander-schranz commented Oct 30, 2023

Very intersting about MobX is bringing decorators back which would make the upgrade from MobX 4 to 6 I think a lot easier: mobxjs/mobx#3638

The main difference seems to be it is now:

-@observable todos = [];
+@observable accessor todos = [];

@alexander-schranz
Copy link
Member

alexander-schranz commented Mar 25, 2024

Issue with CSS Loader is fixed in #7329 and part of 2.6.0. We so not longer require file-loader or raw-loader packages as that is now build in in webpack.

@alexander-schranz
Copy link
Member

We could maybe give https://github.com/cfaester/enzyme-adapter-react-18 a try to update to react 18.

@alexander-schranz
Copy link
Member

Update: Quicktest, https://github.com/cfaester/enzyme-adapter-react-18 it is not a easy way to use just the adapter / update to react 18. It also looks we first would require to update Jest, which first will require update MobX. So my suggestion would be do.

MobX -> Jest -> React

Maybe on the way we can still move some enzym tests to react testing library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Technical Debt Impacts code quality, no or just small impact on end developers and users
Projects
None yet
Development

No branches or pull requests

3 participants