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

move patch-package from devDependencies to dependencies #2199

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

ajm-ska
Copy link
Collaborator

@ajm-ska ajm-ska commented Jul 1, 2023

Description

This minor package adjustment will hopefully fix issue #2198 (npm installation problems of carta-frontend@4.0.0-beta.1)

patch-package was simply moved from 'devDependencies' to 'dependencies'.

I see a new version of patch-package, v7.0.0 was released 2 months ago. But here I left it as the previous v6.5.1 version just in case it was to introduce new bugs which we do not want so close to the final release.

Hopefully, this will not be too difficult to test @veggiesaurus?
I propose we generate a new carta-frontend npm package and try to install it.
I guess the version would need to be bumped up to v4.0.1-beta.1?

sudo npm install -g --unsafe-perm carta-frontend@4.0.1-beta.1

Then if that installs without error, only the carta-controller version would need to be bumped up too?

@kswang1029 I'm not sure if it requires a changelog entry as the code didn't really change, only the file related to compiling it?

Checklist

For linked issues (if there are):

  • assignee and label added
  • ZenHub issue connection, board status, and estimate updated

For the pull request:

  • reviewers and assignee added
  • ZenHub estimate, milestone, and release (if needed) added
  • e2e test passing / corresponding fix added
  • changelog updated / no changelog update needed
  • no protobuf update needed
  • BackendService unchanged

@ajm-ska
Copy link
Collaborator Author

ajm-ska commented Jul 1, 2023

@veggiesaurus I just thought of a way to test it.

  1. I download the current npm package, wget https://registry.npmjs.org/carta-frontend/-/carta-frontend-4.0.0-beta.1.tgz.
  2. Extracted the archive and made the modification to package.json.
  3. Zipped it up again tar -czvf test.tgz package.
  4. Installed that directly sudo npm install -g --unsafe-perm test.tgz

It installs, but there is an error:

[acdc@localhost ~]$ sudo npm install -g --unsafe-perm test.tgz
[sudo] password for acdc:

> carta-frontend@4.0.0-beta.1 postinstall /usr/local/lib/node_modules/carta-frontend
> patch-package

patch-package 6.5.1
Applying patches...
Error: Patch file found for package react-scripts which is not present at node_modules/react-scripts
---
patch-package finished with 1 error(s).
npm WARN notsup Unsupported engine for usehooks-ts@2.9.1: wanted: {"node":">=16.15.0","npm":">=8"} (current: {"node":"14.21.3","npm":"6.14.18"})
npm WARN notsup Not compatible with your version of node/npm: usehooks-ts@2.9.1
npm WARN usehooks-ts@2.9.1 requires a peer of react@^16.8.0  || ^17.0.0 || ^18.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN usehooks-ts@2.9.1 requires a peer of react-dom@^16.8.0  || ^17.0.0 || ^18.0.0 but none is installed. You must install peer dependencies yourself.

+ carta-frontend@4.0.0-beta.1
added 53 packages from 36 contributors in 10.959s

Perhaps something else is needed?

@ajm-ska
Copy link
Collaborator Author

ajm-ska commented Jul 1, 2023

It looks like the above error can be eliminated by also moving react-scripts from devDependencies to dependencies.
I will just make a commit with that change for now.

@kswang1029
Copy link
Collaborator

@ajm-asiaa I think no Changelog is needed.

Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

@ajm-asiaa I tried

npm install --legacy-peer-deps
npm run build-libs
npm run build

and it works as usual. No regression from e2e tests. 👍

Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang left a comment

Choose a reason for hiding this comment

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

This should be a bug caused by my package upgrade PR. Thanks for fixing this!

@veggiesaurus veggiesaurus merged commit d3d12a6 into dev Jul 3, 2023
8 checks passed
@veggiesaurus veggiesaurus deleted the ajm/patch-package-fix branch July 3, 2023 14:30
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