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

feat: provide API to load modules from npm registry #498

Merged
merged 14 commits into from
Aug 28, 2022

Conversation

antongolub
Copy link
Collaborator

@antongolub antongolub commented Aug 24, 2022

  • Tests pass
  • Appropriate changes to README are included in PR

@antonmedv
Copy link
Collaborator

Awesome! Looks super cool. But let’s try to simplify a little bit. I have a few ideas 💡

package.json Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/internals.ts Outdated
return {}
}

await $`npm install --no-save --no-audit --no-fund ${flags} ${pkgs}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool 😎

src/internals.ts Outdated Show resolved Hide resolved
@antongolub antongolub changed the title feat: provide deps API to load modules from npm registry feat: provide API to load modules from npm registry Aug 25, 2022
@pomdtr pomdtr mentioned this pull request Aug 27, 2022
3 tasks
src/cli.ts Outdated
@@ -142,7 +142,7 @@ async function writeAndImport(
if (argv.install) {
await installDeps(parseDeps(contents), {
prefix: dirname(filepath),
registry: argv.registy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can touse be passed via env args? To not pollute argv?

Copy link
Collaborator Author

@antongolub antongolub Aug 27, 2022

Choose a reason for hiding this comment

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

ZX_REGISTRY / ZX_USERCONFIG?
or std npm_config_*?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome 😎

@antongolub antongolub force-pushed the add-deps-api branch 2 times, most recently from 8227814 to 9d6ca3a Compare August 28, 2022 06:46
@antonmedv
Copy link
Collaborator

I’m going to update dev branch, lets merge there.

@antonmedv antonmedv changed the base branch from main to dev August 28, 2022 10:08
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.

2 participants