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

Add support for custom .npmrc #811

Closed
wants to merge 2 commits into from

Conversation

flaviouk
Copy link

@flaviouk flaviouk commented Aug 21, 2020

I was building a modified version of TSDX (custom themes), and one of the issues I faced was that if templates had private packages, even though the template had a correct .npmrc the install step wouldn't pick it up and timeout, the solution was to use shelljs instead.

This would go hand in hand with #502 as well.

@vercel
Copy link

vercel bot commented Aug 21, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/tsdx/nqe6ek8zr
✅ Preview: https://tsdx-git-fork-imflavio-feat-support-npmrc.formium.vercel.app

@agilgur5 agilgur5 added the scope: templates Related to an init template, not necessarily to core (but could influence core) label Aug 21, 2020
const args = getInstallArgs(cmd, deps).join(' ');
await shell.exec(`${cmd} ${args}`, {
silent: true,
});
Copy link
Collaborator

@agilgur5 agilgur5 Aug 21, 2020

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

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

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

@agilgur5 agilgur5 added problem: stale Issue has not been responded to in some time scope: upstream Issue in upstream dependency labels Sep 12, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 18, 2020

So I was looking at CRA issues/PRs to potentially better understand the case for #502 and I found this issue regarding private registries: facebook/create-react-app#8425

That issue is slightly different in that it's about installing a private template, not just private deps, but it's the same concept.
There they seem to recommend using a global ~/.npmrc.

Then when I thought about it, I remembered a template shouldn't include its own .npmrc, because it contains secrets. So I think using a global ~/.npmrc makes more sense anyway. And further to that, generally templates are installed by users and not by robots, so you can just npm login + yarn login beforehand and think you might not need any npmrc then.

@flaviouk
Copy link
Author

Using tsdx directly and copy pasting templates, instead of maintaining a custom tsdx.

@flaviouk flaviouk closed this Sep 19, 2020
@flaviouk flaviouk deleted the feat/support-npmrc branch September 19, 2020 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
problem: stale Issue has not been responded to in some time scope: templates Related to an init template, not necessarily to core (but could influence core) scope: upstream Issue in upstream dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants