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

GN-4543: Fix/enable dependency-lint #159

Merged
merged 1 commit into from
Oct 2, 2023
Merged

GN-4543: Fix/enable dependency-lint #159

merged 1 commit into from
Oct 2, 2023

Conversation

dkozickis
Copy link
Contributor

@dkozickis dkozickis commented Sep 28, 2023

Overview

GN-4542: Fix issues reported by dependency-lint

GN-4542: Enable dependency-lint on CI

Connected issues and PRs:

https://binnenland.atlassian.net/browse/GN-4543

Setup

  1. Checkout

How to test/reproduce

  1. Run ember dependency-lint, should return 0

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@dkozickis dkozickis changed the title GN-4542: Fix/enable dependency-lint GN-4543: Fix/enable dependency-lint Sep 28, 2023
Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Seems like a good solution and the dependency-lint issues are resolved.
There is something I do not understand though:

  • ember-rdfa-editor specifies ~1.0.1 for the ember-focus-trap dependency, so accepts 1.0.1 and 1.0.2
  • appuniversum specifies ^1.0.2 for the ember-focus-trap dependency, so accepts 1.0.2 and 1.1.0 where 1.1.0 is faulty.
    => Why doesn't npm dedupe the dependency and install 1.0.2 once?

@dkozickis
Copy link
Contributor Author

Good question, I don't know 🤔 I've bumped appuniversum yesterday, and it picked 1.1.0 as dependency. I've tried to reinstall it now and it still picks 1.1.0. I am afraid to go down the rabbit hole of resolutions :D

@piemonkey
Copy link
Contributor

piemonkey commented Sep 29, 2023

Hmm, I wonder if using npm install --prefer-dedupe should be the default for any Ember apps and whether it would help here. Unfortunately it doesn't look like we can specify it in package.json, it would be fixed in the lockfile, but someone running npm i without the flag might get local changes. Actually, maybe it would make sense to add npm dedup as a post install script?

@dkozickis
Copy link
Contributor Author

dkozickis commented Sep 29, 2023

Thanks! Did not know about --prefer-dedupe and npm dedupe

Although, using

npm uninstall @appuniversum/ember-appuniversum && npm install @appuniversum/[email protected] --save-dev --prefer-dedupe

still brings [email protected] for @appunivesrum 🤔

P.S. I guess it does not do one off dedupe for a package, but only does a full dedupe, trying with npm dedupe now, and it actually deduped ember-focus-trap

❯ npm ls ember-focus-trap
[email protected] /Users/denis/code/flem/frontend-embeddable-notule-editor
├─┬ @appuniversum/[email protected]
│ └── [email protected]
└─┬ @lblod/[email protected]
  └── [email protected] deduped

it did dedupe the whole package.json though, so there are multiple changes in package-lock.json 😅

@elpoelma
Copy link
Contributor

Thanks! Did not know about --prefer-dedupe and npm dedupe

Although, using

npm uninstall @appuniversum/ember-appuniversum && npm install @appuniversum/[email protected] --save-dev --prefer-dedupe

still brings [email protected] for @appunivesrum 🤔

P.S. I guess it does not do one off dedupe for a package, but only does a full dedupe, trying with npm dedupe now, and it actually deduped ember-focus-trap

❯ npm ls ember-focus-trap
[email protected] /Users/denis/code/flem/frontend-embeddable-notule-editor
├─┬ @appuniversum/[email protected]
│ └── [email protected]
└─┬ @lblod/[email protected]
  └── [email protected] deduped

it did dedupe the whole package.json though, so there are multiple changes in package-lock.json 😅

Ok nice :) I don't think it's a problem that there are other changes in the package-lock

@piemonkey
Copy link
Contributor

Hmm, as I feared, running npm i gives me changes to the package-lock.json. I tried adding a postinstall but npm dedupe is way too slow to be practical. Actually, the changes to package-lock.json were just some packages losing the "dev": true flag for some reason... Still npm dedupe is definitely too slow to be used in a postinstall, so we shouldn't go down that route.

@elpoelma
Copy link
Contributor

I guess we can indeed leave the override on ~1.0.1 for now, with the ember 4 upgrade we'll be able to go to 1.1 without issues.

@elpoelma elpoelma self-requested a review September 29, 2023 14:46
@elpoelma
Copy link
Contributor

I just set the override to ~1.0.1 (which is somewhat less restrictive) and updated ember-focus-trap to 1.0.2 in the package-lock (which contains some fixes)

@abeforgit
Copy link
Member

@dkozickis just needs a merge from latest master and then its good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants