Fix webpacker issues #8136

leio10 · 2021-06-15T10:07:26Z

🎩 What? Why?

This PR fixes all git paths for npm packages and updates the docs with some recent changes.

📌 Related Issues

Link your PR to an issue

Related to #?
Fixes #?

Testing

Describe the best way to test or validate your PR.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

📷 Screenshots

Please add screenshots of the changes you're proposing

♥️ Thank you!

ahukkanen · 2021-06-15T10:26:30Z

@leio10 Remember the packages folder is duplicated in the design app, so when you do changes in it, do the following:

$ rm -rf decidim_app-design/packages
$ cp -R packages decidim_app-design/

ahukkanen · 2021-06-15T11:05:21Z

decidim-core/lib/tasks/decidim_webpacker_tasks.rake

-            dev: "https://gitpkg.now.sh/mainio/decidim/packages_dev/dev?feature/split-npm-packages",
-            prod: "https://gitpkg.now.sh/mainio/decidim/packages_dev/all?feature/split-npm-packages"
+            dev: "https://gitpkg.now.sh/decidim/decidim/packages_dev/dev",
+            prod: "https://gitpkg.now.sh/decidim/decidim/packages_dev/all"


Branch is missing here, add ?develop to the end of the URLs.

Same thing for the URLs in the package.json files.

leio10 · 2021-06-15T11:16:57Z

decidim-core/lib/tasks/decidim_webpacker_tasks.rake

-        {
-          dev: "https://gitpkg.now.sh/#{github_repo}/packages_dev/dev?#{branch}",
-          prod: "https://gitpkg.now.sh/#{github_repo}/packages_dev/all?#{branch}"
+      if decidim_gemspec.source.is_a?(Bundler::Source::Rubygems)


@ahukkanen With this change I'm trying to simplify things as much as possible. My idea here is: if you are using a gem, it should be released and there should be a released npm package with the exactly same version, it doesn't matter the type of version.

@leio10 Yes, isn't it already so? If the gem source is Rubygems (i.e. a released gem), it will refer to the matching NPM packages.

Or did I get something wrong here?

I'm referring that we are now checking for the .dev suffix to use gitpkg in that case. I'd like to remove that part if possible.

Ah, that is because the specs that are testing building the gems and installing them locally. In those situations, the source is reported to be "Rubygems" but the NPM packages for those versions are not yet available.

Ah, ok, I'm slowly understanding all the issues, thanks! ❤️

leio10 · 2021-06-15T11:19:15Z

decidim-core/lib/tasks/decidim_webpacker_tasks.rake

+      # are installed using file references outside the application root
+      # where the `package.json` is located at. For more information, see:
+      # https://github.com/npm/cli/issues/2339
+      FileUtils.cp_r("#{gem_path}/packages", rails_app_path)


If the gems doesn't come from rubygems, just copy the packages folder from the gem's folder into the app and install it locally.

@ahukkanen What do you think about this approach?

@leio10 I think we discussed this approach already at #8119 (comment).

I think copying the packages folder is justified only when the gem source is a path. If the gems come from GitHub, we should install the NPM packages also from GitHub.

I agree with you, but I don't find a better way to do it when having references to other packages:

Using local references doesn't work.

Using gitpkg is not trustworthy. If you install the gem using the commit 123 you need to be sure that all the other packages are from the same commit. That's why I think that copying the folder is the less worst solution.

In my head, I would separate the NPM packages to live their own life compared to the Decidim gems.

So you would never want to reference a single commit from the Decidim repository or Decidim fork for the NPM packages. I think if you want to customize the NPM packages, you need to make a fork of them yourself and reference that fork where you have updated all the references inside the different packages.

I would think that modifying the NPM package's in so much detail that you would have to reference a single commit shouldn't be necessary most times, at least for the time being. They don't contain any running front-end code for Decidim right now, they are mostly for the "essentials" required to manage the packages with Webpacker.

leio10 · 2021-06-15T11:27:43Z

@ahukkanen I've committed some changes to make some tests using the decidim gem from this branch and see if it works properly. It's not a final proposal, just a try to see if I'm able to understand the problem and find the simplest solution. I've added some comments to explain my changes, please let me know your thoughts about them.

Regarding the gitpkg service, I don't like having two packages folder, but my main concern is the mixing of references:

if you use the package https://gitpkg.now.sh/my_user/decidim/packages_dev/all?my_branch that file will (by default) refer to https://gitpkg.now.sh/decidim/decidim/packages_dev/core?develop, among others.
You could change all the references to point to the same repo and branch, but that implies a lot of extra work and you have to change it again after merging.

Without this option, you can be sure that you are always using the right files.

ahukkanen · 2021-06-15T11:51:59Z

Regarding the gitpkg service, I don't like having two packages folder, but my main concern is the mixing of references:

if you use the package https://gitpkg.now.sh/my_user/decidim/packages_dev/all?my_branch that file will (by default) use https://gitpkg.now.sh/decidim/decidim/packages_dev/core?develop.

You could change all the references to point to the same repo and branch, but that implies a lot of extra work and you have to change it again after merging.

Without this option, you can be sure that you are always using the right files.

I also don't like the packages_dev folder. And I also don't like relying on the gitpkg.now.sh service either.

To solve this, I proposed something at: #8121 (comment)

Here is an example implementation of that:
https://github.com/mainio/decidim-npm-builds

If we want to install the packages from git, it is difficult to get all the references correct. I think a proper solution would be relying only on a single dedicated repository for the NPM builds, no matter if Decidim is installed from a git fork referring some other organization. If they want to fork the NPM packages too, they would fork the decidim-npm-builds repository too.

leio10 · 2021-06-15T12:02:00Z

Here is an example implementation of that:
mainio/decidim-npm-builds

If we want to install the packages from git, it is difficult to get all the references correct. I think a proper solution would be relying only on a single dedicated repository for the NPM builds, no matter if Decidim is installed from a git fork referring some other organization. If they want to fork the NPM packages too, they would fork the decidim-npm-builds repository too.

But with the automated package building would you have the ability to use the npm package for the exact commit that your app is pointing to?
I still see that problem here, that's why I'm not convinced of that alternative.

Copying the package folder is ugly and it could fail if you have a packages folder in your app, but it fixes all our problems and doesn't create new ones, right? At the end is like copying the package.json file, but without overring the app specific dependencies.

ahukkanen · 2021-06-15T13:12:37Z

But with the automated package building would you have the ability to use the npm package for the exact commit that your app is pointing to?
I still see that problem here, that's why I'm not convinced of that alternative.

No, you cannot reference a specific commit that way. Or theoretically you could if we made a build for each and every commit in the Decidim repository but I think that would become ugly pretty soon. And it would come with its own restrictions such as having to wait for the build to become available up to 1-2 hours when the next scheduled run is due (GitHub by the way doesn't guarantee the cron schedule would stick and even if it runs always).

As I mentioned above, I would rather think of the NPM dependencies as their own entity outside of the Decidim gems. So outside of the Decidim repository (where core development is done), you would never want to reference a specific commit for the NPM packages. It should be enough that the NPM package version matches the Decidim version.

Copying the package folder is ugly and it could fail if you have a packages folder in your app, but it fixes all our problems and doesn't create new ones, right? At the end is like copying the package.json file, but without overring the app specific dependencies.

You are right that when copying packages, we wouldn't have the problem of the cross-package references and also we wouldn't have to use the gitpkg service.

But personally I am against this idea because it would require updating the local application's packages folder with git installations. Which I think was the initial problem with the package.json.

I'm not seeing why I would want to reference that single commit at any point regarding the NPM packages. If some package is not up to date, I would just run npm update which would fetch the updated package versions. If I would like to add some package, I would run npm i somepackage within the application. If I would like to change e.g. the browserlist configuration, I would define that directly to the package.json and replace the line that says extends @decidim/browserslist-config with my own definitions. If I don't like Decidim's stylelint/ESLint defaults, I would uninstall the @decidim/dev package. And so forth.

I think we just have slightly different point of view regarding this topic. I think the NPM packages and the Decidim gems are detached from each other, not having to match 100% at any given time. You seem to think the opposite about this.

leio10 · 2021-06-15T13:53:04Z

Yes, I understand your point and agree with it. What concerns me is if it's possible to have issues when installing the gem from Git in your app, even when you are not doing anything weird with npm.

Let me give you an example:

We release Decidim v0.25.
You have an app using the last commit from the release\0.25-stable of the gem that is an alias for develop at that moment. You don't use the released gem because you are updating the app frequently from the stable branch (I know that this happens on many Decidim apps).
After one month, you create a PR in your app. The tests will create the app from scratch, and npm will use the package @decidim/all from the right branch. But @decidim/core will be retrieved from develop, so any changes on that files in the last month could break your existing app.
In other words, the commit used by the gem will not be updated, but the commit used by npm will updated everytime you run npm i, in all the environments.

That's why I want to keep the commit used by npm linked to the commit used by the gem. If you upgrade the gem, you should run decidim:upgrade and that would upgrade the packages folder with the right files.

If you want to detach completely the npm version from the gem version, we could use different version numbers for them, but I can't see a clear path to deal with changes. And we still have the problem that probably each gem version will have different dependencies of the npm versions.

ahukkanen · 2021-06-15T16:33:51Z

@leio10 I think we should change the git branch references for the actual releases.

What I thought in https://github.com/mainio/decidim-npm-builds was that there would be e.g. version/0.25.0 branch in the NPM build repo which could be installed against Decidim's release/0.25-stable. That would obviously track the release/0.25-stable versions of the NPM packages containing cross-references to release/0.25-stable versions.

It already does that for the dev verisons. If you check the branches in that repo, there is one version branch for the 0.25.0-dev version. This would just have to be extended to cover the actual releases when they become available with the NPM packages.

ahukkanen · 2021-06-15T16:38:05Z

And what comes to the CI failing if it installs the packages with npm ci. That can happen if the SHA integrity hash for the NPM package has changed.

This would mean you just have to run npm update and commit a new package-lock.json.

leio10 · 2021-06-16T09:31:51Z

Hi @ahukkanen, I feel that we are a bit stuck here. Maybe we can ask @ferblape and @mrcasals to chime in to have more eyes on this. I'll try to explain very briefly the problem we have and the options that we are considering:

The problem

We added a packages folder to the project to create several npm packages with all the Decidim dependencies to be able to install them without overriding the app package.json. But we need to install the right npm package version related to the version of Decidim that the app is using.

We also have to take into account that the app can use the Decidim gem from a local path (dev and test apps), from the released gems or from a git repository (decidim/decidim#develop or with any other user, repo, branch or commit).

The solutions

We all agree that we should publish an npm package during the release process with the exact same version number than the gem, to be used by the apps using that published gem.

We also agree that if we are using the gem from a local folder the best option is to copy the packages folder into the app folder, because a bug in npm don't allow to install dependencies from folders that are outside the project folder.

The discussion

We are not able to find a solution for the scenario where the gem is installed from a git repository. We developed three ideas:

Install the npm package from the git repository and branch used to install the gem. You cannot install a package from a repo subfolder, as the package.json file should be on the root folder of the repo, so we used an external service. The problem is that some of the packages depend on other packages in the same repo, and the local references don't work when used through this service. So, we added another folder named packages_dev using the same external service to replace the local references. But this adds complexity to the solution and the external references always point to decidim/decidim#develop or need to be changed frequently to be updated to the right repo and branch.
Copy the packages folder to the app, as we are doing with the local folder gem. The problem here is that modifies the app adding a packages folder (the app cannot use a folder with that name) and this folder will be updated on each run of the decidim:upgrade task. It's like the original approach we had, but allowing the app maintainer to add mode npm packages to the app, as the package.json of the app is not overriden by Decidim.
Build npm packages frequently for the develop branch and the released version branches. An example of this is done in mainio/decidim-npm-builds. The problem is the maintainance cost of the new repo (create new branches for each release) and that other branches or repositories will not be supported.

ahukkanen · 2021-06-16T11:57:05Z

@leio10 I came up with another possible way to solve this problem which is implemented here:
https://github.com/mainio/decidim-npm-local

So, what you would need to do with git installations is the following:

$ npm i https://github.com/mainio/decidim-npm-local
$ npm exec decidiminstall .

After this, every time you run npm i or npm ci, it will build the Decidim NPM packages from the decidim gem folder when it is available. If the bundle command does not return any path for the decidim gem, it will use the git repository instead.

After you've run those, it will always install the exact same versions of the NPM packages that contained in the installed decidim gem.

Can you test this yourself and let me know what you think?

leio10 · 2021-06-16T14:56:41Z

Ok, @ahukkanen I like your idea, I'll try it as soon as possible 👍

leio10 · 2021-06-18T16:15:36Z

@ahukkanen Thanks for your great work! ❤️
I've been checking your proposal and it worked like a charm for the dev and test apps. But I already have our staging app working with the packages folder, and I guess that maybe we will need to modify the deployment to make the deployed apps to work (to generate the tmp/.. folder with the package before running npm install). So, I'll merge this PR as it is, and later we can create a new PR with your proposal and find a way to make it work everywere.
Regarding the tool you build, I think it would be nice to fork your repo from decidim and publish it as an NPM package too, are you ok with this?

ahukkanen · 2021-06-18T19:18:21Z

@leio10 Yes, you can fork of course, it's open source as stated in the license. Or create a new repo, as you wish. The idea was that it would be published to NPM and its version number does not have to follow the Decidim releases.

FYI: Once you have run those commands above locally, you won't have to change the deployment in any way. Just commit the updated package.json and package-lock.json and it should work. It is enough that the deployment script runs npm install (which it should already happen when using rails assets:precompile, or actually through yarn).

The install script of the local installer that builds the packs runs every time you run npm install. So as long as you have the local installer as a dependency with git deployments, it should work fine.

ahukkanen · 2021-06-18T19:25:03Z

The only thing that has to change in Decidim is the webpacker install task to install the dependencies using the local installer for git installations. It could be also applied to the file path installations if we want to get rid of the duplicated packages folder.

This might also solve the need to duplicate the packages folder in the design app. But the spec that matches the packages.json files at the root and in the design app would have to be changed (because they would no longer match 100%).

* 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) ...

leio10 added 2 commits June 15, 2021 11:44

doc: remove outdated information

42afe2c

fix: update repository used for npm packages

1fa7e43

ahukkanen reviewed Jun 15, 2021

View reviewed changes

refactor: trying to simplify things

0a6a718

leio10 commented Jun 15, 2021

View reviewed changes

fix: reset packages folder before copying npm packages

009739b

leio10 force-pushed the fix/webpacker-fixes branch from cb26fdf to 13cf3f0 Compare June 16, 2021 09:49

fix: manage when gem considered from rubygem but not released

981b2ff

leio10 force-pushed the fix/webpacker-fixes branch from 13cf3f0 to 981b2ff Compare June 16, 2021 09:55

fix: update app design package.json

924982d

leio10 force-pushed the fix/webpacker-fixes branch from 1759d61 to 924982d Compare June 16, 2021 10:08

chore: use graphql-docs from decidim repo

b68f206

leio10 force-pushed the fix/webpacker-fixes branch from 3ed96b1 to b68f206 Compare June 16, 2021 15:25

fix: remove tilde from css imports

8bda182

leio10 merged commit 9f656b2 into develop Jun 18, 2021

leio10 deleted the fix/webpacker-fixes branch June 18, 2021 16:18

ahukkanen mentioned this pull request Jun 24, 2021

Webpacker: Use local NPM installer instead of duplicating the packages folder #8159

Closed

12 tasks

leio10 added type: fix PRs that implement a fix for a bug target: developer-experience and removed type: fix PRs that implement a fix for a bug labels Jul 23, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix webpacker issues #8136

Fix webpacker issues #8136

leio10 commented Jun 15, 2021

ahukkanen commented Jun 15, 2021

ahukkanen Jun 15, 2021

leio10 Jun 15, 2021 •

edited

ahukkanen Jun 15, 2021

leio10 Jun 15, 2021

ahukkanen Jun 15, 2021

leio10 Jun 15, 2021

leio10 Jun 15, 2021 •

edited

ahukkanen Jun 15, 2021

leio10 Jun 15, 2021

ahukkanen Jun 15, 2021

leio10 commented Jun 15, 2021 •

edited

ahukkanen commented Jun 15, 2021

leio10 commented Jun 15, 2021

ahukkanen commented Jun 15, 2021

leio10 commented Jun 15, 2021

ahukkanen commented Jun 15, 2021 •

edited

ahukkanen commented Jun 15, 2021

leio10 commented Jun 16, 2021 •

edited

ahukkanen commented Jun 16, 2021 •

edited

leio10 commented Jun 16, 2021

leio10 commented Jun 18, 2021

ahukkanen commented Jun 18, 2021

ahukkanen commented Jun 18, 2021

Fix webpacker issues #8136

Fix webpacker issues #8136

Conversation

leio10 commented Jun 15, 2021

🎩 What? Why?

📌 Related Issues

Testing

📋 Checklist

📷 Screenshots

ahukkanen commented Jun 15, 2021

Choose a reason for hiding this comment

leio10 Jun 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leio10 Jun 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leio10 commented Jun 15, 2021 • edited

ahukkanen commented Jun 15, 2021

leio10 commented Jun 15, 2021

ahukkanen commented Jun 15, 2021

leio10 commented Jun 15, 2021

ahukkanen commented Jun 15, 2021 • edited

ahukkanen commented Jun 15, 2021

leio10 commented Jun 16, 2021 • edited

The problem

The solutions

The discussion

ahukkanen commented Jun 16, 2021 • edited

leio10 commented Jun 16, 2021

leio10 commented Jun 18, 2021

ahukkanen commented Jun 18, 2021

ahukkanen commented Jun 18, 2021

leio10 Jun 15, 2021 •

edited

leio10 Jun 15, 2021 •

edited

leio10 commented Jun 15, 2021 •

edited

ahukkanen commented Jun 15, 2021 •

edited

leio10 commented Jun 16, 2021 •

edited

ahukkanen commented Jun 16, 2021 •

edited