-
Notifications
You must be signed in to change notification settings - Fork 8
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
How are we going to import @node-core/ui-components
?
#236
Comments
Why not publish ui-components ? |
That's an option, but I feel like we are going to be constantly updating that package, so it might be easier to keep it imported from the repository. |
With a CD it's just a git tag to create no needed to create a changelog |
Personally, I'm in favor of adopting support for pnpm, as it doesn't require the website to also use pnpm, and we can just add the following the "@node-core/ui-components": "github:nodejs/nodejs.org#main&path:packages/ui-components" However, I think we should get more opinions before making any decision. |
Would that interfere with us using |
No, we'd just change the npm installation step to a pnpm installation step, but let's cross that bridge if we get to it.
|
it's mean we will need pnpm in node core ? |
We wouldn't need to depend on it, we would just have to assume the user has it installed, which isn't ideal, but every one of our options has a drawback.
|
I think we should publish the package, but not necessarily npm. It can be as an GitHub artifact. https://docs.github.com/en/packages/quickstart |
That could work. Once the I'm gonna leave this issue open until then, just in case anything changes. |
Are you merging the PR today? |
Yes, I don't think anyone else is going to object in the next hour, so I'm merge it now, but I'm going to add a workflow in a second PR, in case it needs a revert, we don't need to revert the entire UI package |
We need to verify that Node.js can publish to the Edit: We do not control the We have a few options:
|
Of those options you gave, the best and simplest one is just renaming the package to |
cc @nodejs/tsc and @nodejs/build looking for guidance here |
That's a 9 years old, outdated comment.
That's wrong, npm does support installing packages from subdirectories. I was there when we implemented it during the npm7 rewrite, although I acknowledge that it doesn't seem to have been properly documented back then. Here's how you use it:
I would suggest one of you to contribute back to the npm docs so that it's easier to you all and the rest of the world to know how to use it in the future. Here are some refs on how to use it from its tests. |
Thanks for chiming in, Ruy! |
Thanks! Sorry about my outdated information, I'll check and PR the npm documentation to make this more clear. |
npm does not support importing packages from subdirectories of git repositories.
According to npm/npm#2974 (comment):
There are third-party solutions like GitPkg that allow this functionality in npm, but I'm not sure we should rely on a third-party tool to handle imports–lest something bad happen.
However, both Yarn (although, yarn requires the website to also use yarn) and pnpm do support this, so we could convert this repository to use one of them instead.
This would also require any project that runs
api-docs-tooling
use the same package manager (i.e.pnpm dlx
instead ofnpx
for execution).Reference: nodejs/nodejs.org#7401 (comment)
The text was updated successfully, but these errors were encountered: