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.

Sign up for GitHub

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

Jump to bottom

chore: reverted optional dev dependencies and removed npm audit on root package #553

Merged
merged 1 commit into from
May 25, 2022
Merged

chore: reverted optional dev dependencies and removed npm audit on root package #553

merged 1 commit into from
May 25, 2022

Conversation

kirrg001
Copy link
Contributor

I've noticed that even if the packages are installed, there is a longer check which slows down npm install locally

@kirrg001 kirrg001 force-pushed the improve/optional-dev-deps-script branch from f010549 to 119980a Compare May 24, 2022 10:20
@kirrg001 kirrg001 requested a review from basti1302 May 24, 2022 10:20
basti1302
basti1302 approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@basti1302 basti1302 left a comment

Choose a reason for hiding this comment

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

LGTM

@kirrg001 kirrg001 force-pushed the improve/optional-dev-deps-script branch from 119980a to a33bfbc Compare May 24, 2022 12:24
@kirrg001
Copy link
Contributor Author

kirrg001 commented May 24, 2022 •
edited

I've just noticed that the script is creating trouble when running npm i $package ($package can be anything), because npm will remove the optional dev dependencies from the lock file, because it can't find the deps in the package.json file.
NPM does not run postinstall for single dependencie insallations, yarn does 😢

No idea how to solve this problem ATM, because we cannot remove the packages from the lock file e.g. otherwise npm will always install these dependencies (CI caching does not work und local as well). As well as npm list is then broken.

If we could simply run npm i after npm i $package, we can solve this problem. But I don't know how without a hook.

@kirrg001 kirrg001 changed the title chore: optimised install-optional-dev-dependencies.sh chore: reverted optional dev dependencies and removed npm audit on root package May 25, 2022
@kirrg001 kirrg001 force-pushed the improve/optional-dev-deps-script branch from 1586a71 to c945345 Compare May 25, 2022 07:43
@kirrg001
Copy link
Contributor Author

We removed optionalDevDependencies and switched back to optionalDependencies.
optionalDependencies did not work previously because npm audit failed.

@basti1302 realized in the morning that we not have to execute npm audit on the root package. WIth this change, we can reuse optionalDependencies.

@kirrg001
chore: reverted optional dev dependencies and removed npm audit on ro…
399db16 
…ot package

reasons:
- `optionalDevDependencies` was a nice idea but we are running into bugs with e.g. `npm install X`
  NPM does not execute script hooks (such as postinstall) on single dependency installations.
  The consequence is that `npm i X` will remove our optionalDevDependencies from the package-lock file.
  This is not ideal, because we need these packages in the lock file for caching locally and on CI.
- We simply decided to remove `npm audit --prod` from the root package and go back to `optionalDependencies`
@kirrg001 kirrg001 force-pushed the improve/optional-dev-deps-script branch from c945345 to 399db16 Compare May 25, 2022 07:55
@kirrg001 kirrg001 merged commit 610f00c into main May 25, 2022
@kirrg001 kirrg001 deleted the improve/optional-dev-deps-script branch May 25, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@basti1302 basti1302 basti1302 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kirrg001 @basti1302