Split NPM dependencies to more granular packages by ahukkanen · Pull Request #8121 · decidim/decidim

ahukkanen · 2021-06-09T14:54:44Z

🎩 What? Why?

There is a discussion whether Decidim should release NPM packages with the webpacker changes at: #7818

After some PRs (#8066, #8094, #8107) this did not seem necessary as we found a way to control the JS dependencies within the repository. However, the current approach with #8094 and #8119 can be extremely slow as it is cloning the whole Decidim repository to node_modules. Faster would be obviously to load the dependencies from NPM.

Another point to support this change came at #8115 where I needed to override config/webpack/base.js which apparently can change during upgrades (as pointed out by @ferblape at #8115 (comment)). Therefore, it would be safer to place these overrides in @decidim/webpacker package than the file that will be automatically updated during upgrades.

This proposes some changes for managing the NPM dependencies to help this transition:

Splitting the Decidim dependencies to NPM packages hosted in the at the repository's packages folder. The following packages are introduced:
- @decidim/core which loads the dependencies needed by the Decidim UIs, such as jQuery, GraphiQL, Leaflet, Browserslist configuration, etc.
- @decidim/dev which loads the dependencies needed in development, such as axe-core for accessibility checks, stylelint configurations, ESLint configurations
- @decidim/elections which loads the dependencies needed by the elections/votings modules
- @decidim/webpacker which loads the dependencies needed for webpacker and to build the assets with webpacker
- @decidim/all which requires all those packages (except for the @decidim/dev package which is added in the devDependencies)
Adding some supporting packages that are marked as dependencies in the main packages listed above:
- @decidim/browserslist-config which ships the Browserslist configuration centrally for Decidim and Decidim based applications - marked as a dependency for @decidim/core
- @decidim/eslint-config which ships the ESLint configuration centrally for Decidim and Decidim based applications - marked as a dependency for @decidim/dev
- @decidim/stylelint-config which ships the Stylelint configuration centrally for Decidim and Decidim based applications - marked as a dependency for @decidim/dev
Leaving the packages needed for testing to the root package.json's devDependencies (all these used to be inside dependencies)

This does not mean that all of the assets would be moved to NPM, this is just the "essentials" that are currently listed in the root package.json. And also to improve things such as the implementation of #8115.

Right now NPM does not support monorepos for git installations (see npm/npm#2974). I also tried to list the dependencies as file dependencies in the root package.json but that failed too when they are installed on external applications (or the test app for instance) because that approach won't allow NPM to install the dependencies listed in each of those @decidim NPM packages (see npm/cli#2339).

As a workaround, I found the gitpkg.now.sh service which allows installing NPM packages from git repository subfolders.

Obviously, it is not meant to stay this way. Eventually these packages should be released to NPM and we should figure out a proper way how we could avoid version conflicts between the gem versions and the NPM package versions. To me it just seems eventually we will need some NPM packages.

It is also installing the packages from our fork right now because those packages do not yet exist at the main repository.

📌 Related Issues

Related to Configure webpacker additional paths and entry points programmatically #8066, Webpacker: Do not override the application's package.json #8094, Fix webpacker asset configs for external apps #8107, Dynamic stylesheet includes with Webpacker #8115, Choose the right version of Decidim's package.json #8119

Testing

Create the decidim test/development app and run it with the webpack dev server.

📋 Checklist

qlty-cloud-legacy

The PR diff size of 15204 lines exceeds the maximum allowed for the inline comments feature.

qlty-cloud-legacy

The PR diff size of 15204 lines exceeds the maximum allowed for the inline comments feature.

qlty-cloud-legacy

The PR diff size of 15204 lines exceeds the maximum allowed for the inline comments feature.

qlty-cloud-legacy

The PR diff size of 15196 lines exceeds the maximum allowed for the inline comments feature.

qlty-cloud-legacy

The PR diff size of 33735 lines exceeds the maximum allowed for the inline comments feature.

qlty-cloud-legacy

The PR diff size of 33757 lines exceeds the maximum allowed for the inline comments feature.

ahukkanen · 2021-06-11T18:18:09Z

I'll leave just few notes regarding this here:

The new folder packages_dev is for development package replacements which contain git references when the Decidim gems are installed from a git repository.
I would like to eventually get rid of this folder but it is there only because of current NPM limitations.
The packages folder is duplicated to the design app because of two reasons:
1. The file path references will not install linked package dependencies if the linked package is outside of the project root (see npm v7 does not install linked packages dependencies npm/cli#2339). So if you ran e.g. npm i ../packages/all from the design app, it would be a reference outside of the design app (one level above) which does not work.
2. The root webpacker spec checks that the package.json and package-lock.json match with the repository root. If there are changes done to any of the packages, they need to be duplicated to both, the root packages and the design app packages.

What we would need to get rid of the duplicated packages folder inside the design app:

NPM should install linked package dependencies from all locations, even when they are outside of the application folder where they are being installed from (see npm v7 does not install linked packages dependencies npm/cli#2339). It seems to be on their roadmap but the issue has not got any attention lately.
If this happens at some point, some changes are probably needed in the root webpacker spec because the package.json's won't match 100% anymore because the referenced folder would be different. Some diffing is needed.

What we would need to get rid of the packages_dev folder at the repository root:

NPM should allow installing a package from a git subfolder (see Allow subdirectories within git repos in npm install npm/npm#2974). Or alternatively each Decidim NPM package should have their own dedicated repository (which would be harder to manage).
NPM should be able to resolve the package references automatically from the Git monorepo when packages are referred using file references (haven't found any issue referring to this)

IF the first one is solved, we can get rid of the gitpkg.now.sh references and refer to the packages directly from the Decidim repository.

IF the second one is solved, we wouldn't need anymore the packages_dev/**/package.json files because the references could be resolved automatically.

Meanwhile, we could setup another repository that automatically builds the Decidim NPM packages so that they can be distributed directly from GitHub. An example of this is available at:
https://github.com/mainio/decidim-npm-builds

qlty-cloud-legacy

The PR diff size of 33775 lines exceeds the maximum allowed for the inline comments feature.

Also, refactor the package references to be local inside the `packages` folder.

Right now `postcss-preset-env` is not compatible with PostCSS 8. Some packages and the @decidim/webpacker package had the PostCSS 8 dependency which caused multiple versions of these packages to be installed within the packages and within the project folder. This caused `postcss-preset-env` to fail. Related issue: csstools/postcss-preset-env#191

After some refactoring, the git NPM package locations changed which caused also a change in this file. Also, the local package installation no longer requires replacing the `all` folder as the references are now all local inside `packages`.

leio10

Awesome job! Just a few doubts and we can merge this.

leio10 · 2021-06-14T15:14:43Z

decidim-core/lib/tasks/decidim_webpacker_tasks.rake

+        if decidim_gemspec.version.to_s =~ /\.dev$/
+          # With the .dev version the package does not exist at NPM yet.
+          {
+            dev: "https://gitpkg.now.sh/mainio/decidim/packages_dev/dev?feature/split-npm-packages",


💬 Should we remove the branch modifier and use develop?

Yes, after this is merged (in another PR).

leio10 · 2021-06-14T15:16:59Z

packages_dev/all/package.json

+    "npm": "^7.7.2"
+  },
+  "dependencies": {
+    "@decidim/browserslist-config": "https://gitpkg.now.sh/mainio/decidim/packages/browserslist-config?feature/split-npm-packages",


💬 Can we remove the branch reference here? Or do you prefer to create a new PR to change it after merging this?

Yes, after this is merged (in another PR).

leio10 · 2021-06-14T15:17:21Z

packages_dev/dev/package.json

+    "npm": "^7.7.2"
+  },
+  "dependencies": {
+    "@decidim/eslint-config": "https://gitpkg.now.sh/mainio/decidim/packages/eslint-config?feature/split-npm-packages",


💬 Same here with the branch reference

Yes, after this is merged (in another PR).

ahukkanen · 2021-06-14T16:53:20Z

@leio10 Sure, we can change the branch modifier but this has to be merged first so that those packages become available in the Decidim main repository.

I was also thinking an alternative how we could get rid of the gitpkg.now.sh references and the packages_dev folder too. It would be possible if there was another repository that would periodically build the NPM packs and push them back to the repository. Here is an example what that could look like (it runs automatically every hour against this PR branch right now):
https://github.com/mainio/decidim-npm-builds

With that, you can now try out running:

$ npm i https://github.com/mainio/decidim-npm-builds/raw/version/0.25.0-dev/builds/decidim-all.tgz

* develop: (47 commits) New Crowdin updates (#8150) Move the webpacker config override to @decidim/webpacker (#8158) Fix admin stylesheet dynamic imports (#8154) Fix session timeout conflicting with remember me (#7467) Allow to create online meetings without an URL (#8152) Fix verification route issues (#8146) Fix dont save timeout path to session (#8142) Fix access to import CSV results in accountability (#8132) Fix user report notification reported user name (#8130) Allow users to comment and delete their own comments (#8072) New Crowdin updates (#8124) Fix webpacker issues (#8136) Add comments in participatory space presentation page stats block (#8034) Split NPM dependencies to more granular packages (#8121) Metric is not shown when value is zero for blocked and reported users (#8117) Add missing templates translations (#8133) Add missing translation for authorization_modals (#8129) Polls in meetings (#8065) Fix flaky test on initiatives (#8128) Filter participants admin (#8104) ...

ahukkanen added 13 commits June 9, 2021 11:11

Move the NPM dependencies to separate packages

b093117

Add the dependencies to the main JS file

301f54c

Change the essentials package name to core

5acaba3

Change the essential dependency to core

5af880a

Create @decidim/all package and mark the dependency to that package

b85d00f

Include the packages folder in the main NPM package

469814c

Format the files array correctly

eca4cb0

Remove the workspaces definition

7d2353e

Mark the decidim dependencies through GitHub

03cd4ed

Move the dependencies to the root package.json and remove @decidim/all

dd74036

Add "@decidim/all" package

75f73be

Update the core package dependencies

5ef5a6a

Install the @decidim/all package instead of Decidim main package.json

9aa25a2

ahukkanen mentioned this pull request Jun 9, 2021

Choose the right version of Decidim's package.json #8119

Closed

12 tasks

ahukkanen added 6 commits June 9, 2021 18:38

Rename NPM @decidim/votings -> @decidim/elections

1058b4c

Rename the NPM dependency @decidim/votings -> @decidim/elections

432d2a9

Update package-lock.json

68709e8

Update @decidim/all in package-lock.json

b00b62b

Add multiple versions of the NPM package definitions and choose correct

c11f0ff

Include the packages folder in the Decidim release

20762d7

qlty-cloud-legacy bot reviewed Jun 10, 2021

View reviewed changes

Fix the reference to the development NPM packages through git

9c2c010

qlty-cloud-legacy bot reviewed Jun 10, 2021

View reviewed changes

Change reference to correct NPM packages through git

5625525

qlty-cloud-legacy bot reviewed Jun 10, 2021

View reviewed changes

ahukkanen marked this pull request as draft June 10, 2021 10:54

ahukkanen added 2 commits June 10, 2021 21:10

Revert the generator changes and determine NPM package from gemspec

969e1d9

Update package-lock.json

8a1f1e8

qlty-cloud-legacy bot reviewed Jun 10, 2021

View reviewed changes

Fix the design app webpacker spec

721e427

Match the design app package.json with root

bdb48c9

qlty-cloud-legacy bot reviewed Jun 11, 2021

View reviewed changes

Apply the changes from decidim#8119 regarding the upgrade task

7a83cfa

qlty-cloud-legacy bot reviewed Jun 11, 2021

View reviewed changes

ahukkanen marked this pull request as ready for review June 11, 2021 17:51

ahukkanen added 2 commits June 12, 2021 13:43

Move the @decidim/dev package to devDependencies

86b8a08

Merge branch 'develop' into feature/split-npm-packages

24ad6ff

qlty-cloud-legacy bot reviewed Jun 12, 2021

View reviewed changes

ahukkanen added 5 commits June 12, 2021 17:53

Move ESLint/Stylelint/Browserlist configs to @decidim packages

97aa350

Also, refactor the package references to be local inside the `packages` folder.

Add the decidim browserslist configuration to the application

c8853b0

Remove unnecessary entry inside the files node of package.json

8de1c41

leio10 added in-review target: developer-experience type: enhancement labels Jun 14, 2021

ahukkanen added 2 commits June 14, 2021 16:17

Update the NPM package authors field

02ad013

Merge branch 'develop' into feature/split-npm-packages

3c8ece4

leio10 approved these changes Jun 14, 2021

View reviewed changes

leio10 merged commit a2c340b into decidim:develop Jun 15, 2021

ahukkanen mentioned this pull request Jun 15, 2021

Fix webpacker issues #8136

Merged

12 tasks

ahukkanen deleted the feature/split-npm-packages branch June 22, 2021 09:37

ahukkanen mentioned this pull request Jun 24, 2021

Move the webpacker config override to @decidim/webpacker #8158

Merged

12 tasks

andreslucena added type: internal PRs that aren't necessary to add to the CHANGELOG for implementers and removed type: enhancement target: developer-experience labels Jan 10, 2022

ahukkanen mentioned this pull request Feb 19, 2024

Remove uuid NPM package as a core dependency #12482

Merged

Uh oh!

Conversation

ahukkanen commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

Testing

📋 Checklist

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

ahukkanen commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

Uh oh!

leio10 left a comment

Choose a reason for hiding this comment

Uh oh!

leio10 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

ahukkanen Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

leio10 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

ahukkanen Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

leio10 Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

ahukkanen Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

ahukkanen commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahukkanen commented Jun 9, 2021 •

edited

Loading

ahukkanen commented Jun 11, 2021 •

edited

Loading

ahukkanen commented Jun 14, 2021 •

edited

Loading