Skip to content

feat: add lockfile-include-tarball-url option#5054

Merged
zkochan merged 5 commits into
pnpm:mainfrom
MBelniak:add-save-tarball-url
Jul 20, 2022
Merged

feat: add lockfile-include-tarball-url option#5054
zkochan merged 5 commits into
pnpm:mainfrom
MBelniak:add-save-tarball-url

Conversation

@MBelniak

@MBelniak MBelniak commented Jul 18, 2022

Copy link
Copy Markdown
Contributor

In this PR I add lockfile-include-tarball-url option, which when set to true on npm env, makes the installation process of new packages add their resolved tarball URL to pnpm-lock.yaml

I need a way to save a registry from which each package was fetched in pnpm-lock.yaml.
In my company we have 2 registries - one with officially approved packages, the other one mirroring the public npm registry, for development purposes.
I want to keep the information which registry was used for a given package in pnpm-lock.yaml, so that during PR review we know, if there was a package fetched from unofficial registry and it should be first approved.
I see that sometimes the tarball URL is saved, but only if the url is non-standard.
For my case it's really helpful to keep that information for each package.

@MBelniak MBelniak requested a review from zkochan as a code owner July 18, 2022 12:30
@welcome

welcome Bot commented Jul 18, 2022

Copy link
Copy Markdown

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

@MBelniak

MBelniak commented Jul 18, 2022

Copy link
Copy Markdown
Contributor Author

I had a trouble running tests using pnpm --filter=@pnpm/plugin-commands-installation test, because I don't know how to setup a mock registry. I kept getting an error:

GET http://localhost:7774/is-negative: Not Found - 404

So if anyone could guide me on this I would be grateful.

@zkochan

zkochan commented Jul 18, 2022

Copy link
Copy Markdown
Member

I don't know how to setup a mock registry.

that command should run the registry. Try again.

The setting name is not very good. I'd probably call it something like lockfile-include-tarball-url

Add a test and a changeset.

@MBelniak MBelniak changed the title feat: add save-tarball-url feat: add lockfile-include-tarball-url option Jul 19, 2022
@MBelniak

MBelniak commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

I don't know how to setup a mock registry.

that command should run the registry. Try again.

The setting name is not very good. I'd probably call it something like lockfile-include-tarball-url

Add a test and a changeset.

The registry is set up, but there are no is-positive and is-negative packages there, thus 404. I couldn't find a fix, so I will rely on CI tests.

As requested, I added a test and a changeset and I changed option name to the one suggested. New test passed locally. I hope it is OK.
Let me know if there is anything more I should do.

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

/home/work/pnpm/packages/fetcher-base/src/index.ts
  3:1  error  'ssri' should be listed in the project's dependencies. Run 'npm i -S ssri' to add it  import/no-extraneous-dependencies

@zkochan

zkochan commented Jul 19, 2022

Copy link
Copy Markdown
Member

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

Did you run pnpm install?

@MBelniak

Copy link
Copy Markdown
Contributor Author

Also, FYI for each git push I got the error below, so I had to push with --no-verify, because this is not related to my change.

Did you run pnpm install?

Yes, I did.

MBelniak added 4 commits July 19, 2022 15:31
- add save-tarball-url option, which saves resolved tarball URL to
pnpm-lock.yaml during install or add command
…dd test

- option is now named lockfile-include-tarball-url as suggested by
@zkochan
- add test covering new feature
@zkochan zkochan force-pushed the add-save-tarball-url branch from 7da48d5 to 4f854ee Compare July 19, 2022 12:31
Comment thread packages/plugin-commands-installation/test/add.ts Outdated
@zkochan zkochan requested a review from a team July 20, 2022 00:29
@zkochan zkochan changed the title feat: add lockfile-include-tarball-url option feat: add lockfile-include-tarball-url option Jul 20, 2022
@MBelniak

Copy link
Copy Markdown
Contributor Author

Thanks for the last commit :) So, I guess now I just wait for your approval and merge.

@zkochan zkochan merged commit 406656f into pnpm:main Jul 20, 2022
@welcome

welcome Bot commented Jul 20, 2022

Copy link
Copy Markdown

Congrats on merging your first pull request! 🎉🎉🎉

@MBelniak

Copy link
Copy Markdown
Contributor Author

Wow, I didn't expect it will be so quick :D Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants