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

Update npm dependencies #38041

Closed

Conversation

roland-d
Copy link
Contributor

@roland-d roland-d commented Jun 12, 2022

Summary of Changes

This is the result of a npm update to update our dependencies to their current versions as supported by our PHP minimum standard.

@brianteeman I cannot update the skipto plugin to anything higher than 4.1.3 because the newer versions are missing the compiled folder. Is that something you can look into?

The output of npm outdated before

Package                        Current  Wanted  Latest  Location                                    Depended by
@babel/core                     7.16.7  7.18.2  7.18.2  node_modules/@babel/core                    joomlacms.test
@babel/preset-env               7.16.7  7.18.2  7.18.2  node_modules/@babel/preset-env              joomlacms.test
@fortawesome/fontawesome-free   5.15.4  5.15.4   6.1.1  node_modules/@fortawesome/fontawesome-free  joomlacms.test
@popperjs/core                  2.11.0  2.11.5  2.11.5  node_modules/@popperjs/core                 joomlacms.test
@rollup/plugin-babel             5.3.0   5.3.1   5.3.1  node_modules/@rollup/plugin-babel           joomlacms.test
@rollup/plugin-commonjs         21.0.1  21.1.0  22.0.0  node_modules/@rollup/plugin-commonjs        joomlacms.test
@rollup/plugin-node-resolve     13.1.2  13.3.0  13.3.0  node_modules/@rollup/plugin-node-resolve    joomlacms.test
@rollup/plugin-replace           3.0.1   3.1.0   4.0.0  node_modules/@rollup/plugin-replace         joomlacms.test
@vue/compiler-sfc               3.2.26  3.2.37  3.2.37  node_modules/@vue/compiler-sfc              joomlacms.test
accessibility                   3.0.15  3.0.16  3.0.16  node_modules/accessibility                  joomlacms.test
autoprefixer                    10.4.1  10.4.7  10.4.7  node_modules/autoprefixer                   joomlacms.test
choices.js                       9.1.0   9.1.0  10.1.0  node_modules/choices.js                     joomlacms.test
chokidar                         3.5.2   3.5.3   3.5.3  node_modules/chokidar                       joomlacms.test
codemirror                      5.65.2  5.65.5   6.0.0  node_modules/codemirror                     joomlacms.test
commander                        8.3.0   8.3.0   9.3.0  node_modules/commander                      joomlacms.test
core-js                         3.20.1  3.22.8  3.22.8  node_modules/core-js                        joomlacms.test
cssnano                         5.0.14  5.1.11  5.1.11  node_modules/cssnano                        joomlacms.test
diff                             5.0.0   5.1.0   5.1.0  node_modules/diff                           joomlacms.test
eslint                           8.5.0  8.17.0  8.17.0  node_modules/eslint                         joomlacms.test
eslint-plugin-import            2.25.3  2.26.0  2.26.0  node_modules/eslint-plugin-import           joomlacms.test
eslint-plugin-vue                8.2.0   8.7.1   9.1.1  node_modules/eslint-plugin-vue              joomlacms.test
fs-extra                        10.0.0  10.1.0  10.1.0  node_modules/fs-extra                       joomlacms.test
ini                              2.0.0   2.0.0   3.0.0  node_modules/ini                            joomlacms.test
jasmine-core                    3.10.1  3.99.1   4.2.0  node_modules/jasmine-core                   joomlacms.test
jquery-migrate                   3.3.2   3.4.0   3.4.0  node_modules/jquery-migrate                 joomlacms.test
karma                           6.3.17  6.3.20  6.3.20  node_modules/karma                          joomlacms.test
karma-coverage                   2.1.0   2.2.0   2.2.0  node_modules/karma-coverage                 joomlacms.test
karma-jasmine                    4.0.1   4.0.2   5.0.1  node_modules/karma-jasmine                  joomlacms.test
karma-verbose-reporter           0.0.6   0.0.6   0.0.8  node_modules/karma-verbose-reporter         joomlacms.test
mediaelement                     5.0.4   5.0.5   5.0.5  node_modules/mediaelement                   joomlacms.test
postcss                          8.4.5  8.4.14  8.4.14  node_modules/postcss                        joomlacms.test
postcss-scss                     4.0.2   4.0.4   4.0.4  node_modules/postcss-scss                   joomlacms.test
rollup                          2.62.0  2.75.6  2.75.6  node_modules/rollup                         joomlacms.test
sass                            1.49.9  1.52.3  1.52.3  node_modules/sass                           joomlacms.test
selenium-standalone              8.0.8   8.1.4   8.1.4  node_modules/selenium-standalone            joomlacms.test
skipto                           4.1.3   4.1.6   4.1.6  node_modules/skipto                         joomlacms.test
stylelint                       14.2.0  14.9.1  14.9.1  node_modules/stylelint                      joomlacms.test
stylelint-config-standard       24.0.0  24.0.0  26.0.0  node_modules/stylelint-config-standard      joomlacms.test
stylelint-scss                   4.1.0   4.2.0   4.2.0  node_modules/stylelint-scss                 joomlacms.test
terser                           5.7.0  5.14.1  5.14.1  node_modules/terser                         joomlacms.test
tinymce                         5.10.3  5.10.5   6.0.3  node_modules/tinymce                        joomlacms.test
vue                             3.2.26  3.2.26  3.2.37  node_modules/vue                            joomlacms.tes

The output of npm outdated after

Package                        Current  Wanted  Latest  Location                                    Depended by
@fortawesome/fontawesome-free   5.15.4  5.15.4   6.1.1  node_modules/@fortawesome/fontawesome-free  joomlacms.test
@rollup/plugin-commonjs         21.1.0  21.1.0  22.0.0  node_modules/@rollup/plugin-commonjs        joomlacms.test
@rollup/plugin-replace           3.1.0   3.1.0   4.0.0  node_modules/@rollup/plugin-replace         joomlacms.test
choices.js                       9.1.0   9.1.0  10.1.0  node_modules/choices.js                     joomlacms.test
codemirror                      5.65.5  5.65.5   6.0.0  node_modules/codemirror                     joomlacms.test
commander                        8.3.0   8.3.0   9.3.0  node_modules/commander                      joomlacms.test
eslint-plugin-vue                8.7.1   8.7.1   9.1.1  node_modules/eslint-plugin-vue              joomlacms.test
ini                              2.0.0   2.0.0   3.0.0  node_modules/ini                            joomlacms.test
jasmine-core                    3.99.1  3.99.1   4.2.0  node_modules/jasmine-core                   joomlacms.test
karma-jasmine                    4.0.2   4.0.2   5.0.1  node_modules/karma-jasmine                  joomlacms.test
karma-verbose-reporter           0.0.6   0.0.6   0.0.8  node_modules/karma-verbose-reporter         joomlacms.test
skipto                           4.1.3   4.1.3   4.1.6  node_modules/skipto                         joomlacms.test
stylelint-config-standard       24.0.0  24.0.0  26.0.0  node_modules/stylelint-config-standard      joomlacms.test
tinymce                         5.10.5  5.10.5   6.0.3  node_modules/tinymce                        joomlacms.test
vue                             3.2.26  3.2.26  3.2.37  node_modules/vue                            joomlacms.test

Testing Instructions

  1. Checkout this branch
  2. Run npm ci in the root of your site
  3. Check if the frontend and backend work as expected.

Actual result BEFORE applying this Pull Request

The site works as expected.

Expected result AFTER applying this Pull Request

The site works as expected.

Documentation Changes Required

None

Signed-off-by: Roland Dalmulder <[email protected]>
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Jun 12, 2022
@brianteeman
Copy link
Contributor

@roland-d there is already a pull request for skipto. Has been here for a long time waiting on merging. There are also pull; requests for codemirror and tinymce

This PR is almost certainly wrong. But it cant be tested/verified because of 11,481 additions, 6,706 deletions

What you are missing is that for some of these packages there is additional steps to be taken which are in the build scripts.

At a minimum you should include a list of the packages that have been updated and the versions.

One way to do this is to run npm outdated before npm update and compare with the output afterwards

@roland-d
Copy link
Contributor Author

@brianteeman Thank you for the feedback. This is my first time updating this thing, so feel free to tell me what is wrong. Yes I am aware that it is basically untestable but not updating isn't an option either. I am fine to close this if someone else wants to pick this up.

This PR is almost certainly wrong.

Is there any info on what is the right way?

What you are missing is that for some of these packages there is additional steps to be taken which are in the build scripts.

I don't have to guess if there is any information on that.

there is already a pull request for skipto. Has been here for a long time waiting on merging.

Found them, going to have a look at it.

One way to do this is to run npm outdated before npm update and compare with the output afterwards

Updated the first post with that information.

@brianteeman
Copy link
Contributor

If you check the joomla build scripts you will find that for tinymce, codemirror, jooa11y, accessibility and skipto there are specific things that need to take place when they are updated. (there may be others but those are the ones I am personally aware of In other words npm update is only the first part of the process of updating those scripts. You can see this your self by either checking the build scripts or simply run npm i after the update and see the changed files.

Because of this there is specific things that need to be checked for these three such as that the version number in the plugins are bumped etc etc.

For that reason I had created pull requests for each of those so that they could be tested and updated correctly.

tinymce #37949
codemirror #37924
accessibility #38009
skipto #37227

@brianteeman
Copy link
Contributor

Why is this for 4.2? It should be for 4.1

@roland-d
Copy link
Contributor Author

That is because 4.1-dev is closed for merging and Benjamin does not expect another 4.1.x release to come out.

I will check all the other PRs later as well.

@brianteeman
Copy link
Contributor

So you think its ok to be releasing joomla with outdated scripts - how sad.

@roland-d
Copy link
Contributor Author

LOL, it is really not about what I think. I don't know how you reached the conclusion about what I think, I am just stating the situation and has nothing to do with what I think. I can tell you what I think and that is that we should always ship with the most up-to-date versions possible. If I really would think what you think I am thinking then I would not make the effort for updating the NPM and composer packages.

@brianteeman
Copy link
Contributor

we should always ship with the most up-to-date versions possible.

that's all anyone can ask for

@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

Just that you understand a bit more the context why Roland is doing this updates. We have been discussing in the last maintainers meeting when would be the best time to update our dependencies regularly. We have been decided to do that before the first beta normally, but for 4.2 before the second beta. Here is the link to the report https://volunteers.joomla.org/teams/cms-maintenance-team/reports/1744-meeting-notes-june-08-2022.

@brianteeman
Copy link
Contributor

Thanks for the info @laoneo

@brianteeman
Copy link
Contributor

That practice is OK for scripts that are simple dependencies but doesnt work when the script itself is the extension. eg skipto, tinymce, codemirorr

@laoneo
Copy link
Member

laoneo commented Jun 13, 2022

In theory, when the script follows semver, then it should be fine to update when we not do a major bump of it.

@brianteeman
Copy link
Contributor

and that's all I have asked for. Yet the pull requests I created for these exact scenarios were all ignored for 4.1 and now flagged as being for 4.2

@brianteeman
Copy link
Contributor

Now that you have updated tinymce etc this will need redoing. From experience it is easier to recreate the pr than to try and resolve the conflicts

@brianteeman
Copy link
Contributor

Would you like me to do it for you?

@roland-d
Copy link
Contributor Author

@brianteeman If you can please.

@brianteeman
Copy link
Contributor

no problem - close this here and I will create a new pr

@roland-d
Copy link
Contributor Author

Thank you.

@roland-d roland-d closed this Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants