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(learn): add article for publishing a typescript package #7279

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

@JakobJingleheimer JakobJingleheimer commented Nov 23, 2024

This is not ready for feedback

Description

Document the recommended way to publish a typescript package

Validation

Related Issues

nodejs/typescript#19

Depends on #7229

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Copy link

vercel bot commented Nov 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Dec 30, 2024 0:04am

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Dec 29, 2024

Jacob you should:

fail-fast: false # prevent a failure in one version cancelling other runs

steps:
- uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

these version refs will likely get outdated pretty fast; what’s the plan for keeping them updated?

"lint": "…",
"types:check": "tsc --noEmit"
},
"optionalDependencies": {
Copy link
Member

@ljharb ljharb Dec 29, 2024

Choose a reason for hiding this comment

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

this is incorrect; optional deps are runtime deps, and will always be installed - it just won’t fail the overarching install if it fails to compile. This will definitely install typescript locally, and for all consumers. There is no way to have optional dev deps, and “optional” wouldn’t be the same as it means for peerDependenciesMeta.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible via --omit, I just haven't got to that yet :)

Copy link
Member

Choose a reason for hiding this comment

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

For an application, sure, but then doesn't that force consumers to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, why? As you say, it won't do anything magical on its own.

Copy link
Member

Choose a reason for hiding this comment

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

Anything in optionalDependencies will automatically be installed for consumers precisely the same as if it's in dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes…? Exactly?

Copy link
Member

@ljharb ljharb Dec 30, 2024

Choose a reason for hiding this comment

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

and why would typescript be a runtime dependency? it should only ever be a peerDependency of a package, at most, and a package that isn't directly about typescript shouldn't be requiring consumers to install typescript at all.

Copy link
Member

Choose a reason for hiding this comment

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

@JakobJingleheimer Can we change it to devDependencies instead?

apps/site/pages/en/learn/typescript/publishing.md Outdated Show resolved Hide resolved
- name: Publish with provenance
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}
run: npm publish --access public --provenance
Copy link
Member

Choose a reason for hiding this comment

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

this publishes with one factor, which is much less secure than publishing with 2fa; i strongly suggest node not in any way encourage doing this.

Choose a reason for hiding this comment

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

Is there any way to set up automated version publishing with 2FA? That would be ideal - automated and secure.

Copy link
Member

Choose a reason for hiding this comment

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

No, not without an external server - until GitHub adds a native way for a workflow to pause and wait for user input to continue.

Copy link
Member

Choose a reason for hiding this comment

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

Oula, I don't agree with you 100%. First of all you can ask for the 2FA for the provenance depending on the NPM token used. Also I think Node.js should recommend packages with provenance for security reasons.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what you mean - the only way to do two-factor is with two factors. Using an npm token is only ever a single factor.

As for provenance, I'm happy to discuss that separately, but whether it has value on its own or not is irrelevant - having 2FA is universally recognized as table stakes for security, which is why github and npm are making it required for everyone. They're not even planning to ever require provenance information, which should tell you about the relative security value of each.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb could you point me to some docs on the alternative? From googling, I just get instructions on how to set up 2FA on my npm account (which I already have), or use it interactively via CLI. And on the GHA side, I get only info about adding 2FA to my GH account (again, already have it).

But if you say GHA can't wait for something, I don't see how it's possible.

Copy link
Member

Choose a reason for hiding this comment

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

The only secure alternative is to publish locally, unless you have the resources to maintain an external server (like https://github.com/GoogleCloudPlatform/wombat-dressing-room) or unless you want to trust a third party server (like https://github.com/step-security/wait-for-secrets).

Copy link
Member Author

Choose a reason for hiding this comment

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

After asking around, I'm not getting a clear answer here. One security expert said use 2FA to secure the account and provenance to authenticate the release (slightly paraphrased, because it wasn't in English).

This seems logical and sufficient to me because you can't do the things triggered with the token without first passing the 2FA on the account doing them.

@nodejs/security could you please advise? Both things seem important yet seemingly cannot be combined during a publish step.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, to do both is impossible at the moment due to a limitation of all CI systems I'm aware of, and because npm provenance only works from a "trusted" CI system. It's possible GitLab offers this capability, but since Github doesn't, I'm not sure how it makes sense to recommend users do it.

Copy link
Member

Choose a reason for hiding this comment

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

also, the way npm publish/automation tokens work, 2FA is bypassed entirely - that's my point.

@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Dec 29, 2024

Thank you all, but if I may direct your attention to the huge note at the top of the PR 😜

This is not ready for feedback

Feedback is very welcome soon 🙏 but not now. Ex the GHA samples are copy+pasted from another project and have not been updated yet.

Co-authored-by: Augustin Mauroy <[email protected]>
Signed-off-by: Jacob Smith <[email protected]>

- Node runs TypeScript code via a process called "[type stripping](https://nodejs.org/api/typescript.html#type-stripping)", wherein node (via [Amaro](https://github.com/nodejs/amaro)) removes TypeScript-specific syntax, leaving behind vanilla JavaScript (which node already understands). This behaviour is enabled by default of node version 23.6.0.

- Node does **not** strip types in `node_modules` because it can cause significant performance issues for the official TypeScript compiler (`tsc`), so the TypeScript maintainers would like to discourage people publishing raw TypeScript, at least for now.
Copy link
Member

Choose a reason for hiding this comment

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

I would add that it slow down the VSCode language server as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the same reason, right?

Copy link
Member

Choose a reason for hiding this comment

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

yes exactly

},
"optionalDependencies": {
"typescript": "^5.7.2"
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an optionalDependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the reasons discussed in this PR 😅

Copy link
Member

Choose a reason for hiding this comment

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

}
```

Pro-tip: The TypeScript executable (`tsc`) is likely used only in CI. Avoid bloating your local node_modules (where you probably won't use it) by adding [`--omit="optional"`](https://docs.npmjs.com/cli/v11/commands/npm-install#omit) when you run `npm install` locally: `npm install --omit="optional"`
Copy link
Member

Choose a reason for hiding this comment

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

This is actually not something I would recommend. Move typescript to a devDependency

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would you not recommend?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, optional is something not needed for the project. Dev is used for dev part and classic for runtime. So to transpile we need to put it in dev

Copy link
Member

Choose a reason for hiding this comment

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

Exactly that. Otherwise ts will be installed every time.

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should advice it as devDependency

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.

7 participants