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

Add support for custom .npmrc #811

Closed
wants to merge 2 commits into from
+4 −1
Closed

Add support for custom .npmrc #811

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9299c0
Add support for custom .npmrc
flaviouk Aug 21, 2020
fc9f789
Fix lint
flaviouk Aug 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ prog
const installSpinner = ora(Messages.installing(deps.sort())).start();
try {
const cmd = await getInstallCmd();
await execa(cmd, getInstallArgs(cmd, deps));
const args = getInstallArgs(cmd, deps).join(' ');
await shell.exec(`${cmd} ${args}`, {
silent: true,
});
Copy link
Collaborator

@agilgur5 agilgur5 Aug 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that these are functionally equivalent. execa has an option to use a shell as well

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's what I thought, but something funky must be going on, if you add an .npmrc to one of the templates pointing to a private npm registry and include a dependency from that private registry, you'll see that execa doesn't work at all

Copy link
Author

Choose a reason for hiding this comment

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

@agilgur5 await execa(cmd, getInstallArgs(cmd, deps), { shell: true }) does not work

Copy link
Author

Choose a reason for hiding this comment

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

@agilgur5 Any update? Can this go in or what changes should I make? Would rather not maintain a fork of TSDX

Copy link
Collaborator

@agilgur5 agilgur5 Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't expect an immediate response only 24 hours after you took 5 days to respond to a comment... I pay companies for premium support and don't get a response that fast, but you're asking a volunteer to respond faster than that and than you yourself did, for a private registry feature at that.
That's counterproductive and does the opposite of motivating maintainers.

Would rather not maintain a fork of TSDX

It sounds like you already are if you have custom templates??? You said yourself "I was building a modified version of TSDX"...
tsdx create does not create an .npmrc and all deps it installs are public, so there's no need for one, except in a fork

We have also generally recommend patch-package when one needs further customization and this is a good use-case for it

Copy link
Collaborator

@agilgur5 agilgur5 Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It sounds like this is an upstream issue in execa and so I think an issue should be filed there. I looked there and there actually is an existing issue on this exact topic: sindresorhus/execa#380

The fix there is to use cwd. If that resolves it, I think it would be a much better option than using shelljs which has various explicit caveats to its usage (several listed in execa's shell option that I linked previously). If it doesn't, I'd suggest filing an issue upstream

installSpinner.succeed('Installed dependencies');
console.log(await Messages.start(pkg));
} catch (error) {
Expand Down