Skip to content

Remove npm decidim packages with dependencies from other decidim packages#8330

Merged
leio10 merged 11 commits intodevelopfrom
chore/fix-npm-packages
Sep 16, 2021
Merged

Remove npm decidim packages with dependencies from other decidim packages#8330
leio10 merged 11 commits intodevelopfrom
chore/fix-npm-packages

Conversation

@leio10
Copy link
Copy Markdown
Contributor

@leio10 leio10 commented Sep 14, 2021 •
edited
Loading

🎩 What? Why?

When using decidim npm packages from the npm repository, the installation fails to detect all the dependencies, because the packages all and dev use relative paths to refer to the other packages.

This PR removes the all package and adds the dependencies to the needed packages directly in the application.

📌 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.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@leio10 leio10 requested a review from andreslucena September 14, 2021 17:03
@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Sep 15, 2021

⚠️ broken CI!

As the exception was with the file decidim_api_docs.js, I thought it was caused by dc8654f, so I dropped it locally and after executing RAILS_ENV=test bundle exec rails assets:precompile I get another similar exception, this time with @decidim/decidim-bulletin_board.graphql:

webpack-cli] ModuleNotFoundError: Module not found: Error: Can't resolve '@decidim/decidim-bulletin_board' in '/home/apereira/Work/decidim/decidim/decidim-elections/app/packs/src/decidim/elections'

@leio10 leio10 force-pushed the chore/fix-npm-packages branch 6 times, most recently from e9db86e to 2f4884b Compare September 15, 2021 15:44
ahukkanen
ahukkanen reviewed Sep 15, 2021
View reviewed changes
decidim-core/lib/tasks/decidim_webpacker_tasks.rake Outdated
prod: local_npm_dependencies_list(package_json["dependencies"]),
dev: local_npm_dependencies_list(package_json["devDependencies"]),
peer: local_npm_dependencies_list(package_json["peerDependencies"]),
optional: local_npm_dependencies_list(package_json["optionalDependencies"])
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen Sep 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leio10 If you do it this way, you can skip the :peer and :optional categories here.

Peer dependencies are used for NPM dependencies such as eslint/stylelint configs to mark the version of the host package they are compatible with.

Optional dependencies could be used e.g. in a situation where you add additional non-necessary functionality to a package, e.g. a package that exports data to CSV, you could add also XLSX format as optional dependency (and functionality available only when the optional dependency is met). Currently there are no optional dependencies within the Decidim's own NPM packages.

Decidim's root package.json does not define either of these and never will. These categories are meant for NPM packages that are pushed to a package repository.

And they should not be added to the application's package.json either.

So, these two lines can be removed.

@leio10 leio10 force-pushed the chore/fix-npm-packages branch from 79a63f6 to 9b3831c Compare September 15, 2021 20:19
@leio10 leio10 force-pushed the chore/fix-npm-packages branch from 6d04111 to 9b674bc Compare September 16, 2021 09:15
@leio10 leio10 mentioned this pull request Sep 16, 2021
Closed
12 tasks
@leio10 leio10 force-pushed the chore/fix-npm-packages branch from dcde75d to b75cb94 Compare September 16, 2021 13:58
@leio10 leio10 force-pushed the chore/fix-npm-packages branch from b75cb94 to ce12acd Compare September 16, 2021 14:38
@leio10 leio10 merged commit bfcd80e into develop Sep 16, 2021
@leio10 leio10 deleted the chore/fix-npm-packages branch September 16, 2021 17:23
This was referenced Sep 17, 2021
Merged
Merged
leio10 added a commit that referenced this pull request Sep 17, 2021
@leio10
Fix problems introduced by #8330 (#8341)
33533a7 
* chore: bump problematic JS dependency version

* fix: replace changed variable name
@alecslupu alecslupu mentioned this pull request Jan 2, 2022
Merged
12 tasks
@andreslucena andreslucena added type: bug type: internal PRs that aren't necessary to add to the CHANGELOG for implementers and removed target: developer-experience type: bug type: fix PRs that implement a fix for a bug labels Jan 10, 2022
@alecslupu alecslupu added this to the 0.26.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahukkanen ahukkanen ahukkanen left review comments

@andreslucena andreslucena Awaiting requested review from andreslucena

Assignees

No one assigned

Labels

module: core type: internal PRs that aren't necessary to add to the CHANGELOG for implementers

Projects

None yet

Milestone

0.26.0

Development

Successfully merging this pull request may close these issues.

4 participants

@leio10 @andreslucena @ahukkanen @alecslupu