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

Create separate npm-packages doc #53

Merged
merged 1 commit into from
Mar 1, 2021
Merged

Create separate npm-packages doc #53

merged 1 commit into from
Mar 1, 2021

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Mar 1, 2021

What does this change?

creates a separate npm-packages doc and links back to it from the client side one.

@gtrufitt gtrufitt requested a review from oliverlloyd March 1, 2021 10:16
@sndrs sndrs merged commit 6f53406 into master Mar 1, 2021
@sndrs sndrs deleted the npm-packages branch March 1, 2021 10:27
## Using

- Make sure you include any `@guardian` packages for transpilation in your build process
- e.g. your [`webpack.config.js`](https://github.com/webpack/webpack/issues/2031#issuecomment-219040479) might including something that looks like this:
Copy link

@oliverlloyd oliverlloyd Mar 1, 2021

Choose a reason for hiding this comment

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

Could we also add examples for tooling such as storybook and jest? Maybe with examples of the sorts of errors that can be expected to be seem if this isn't done?

I've often hit this issue in multiple repos for multiple tools and even though I've fixed it several times it's easy to forget to tell the config to not exclude guardian packages and be faced with confusing error messages. I can imagine other people could have similar problems.

I think the motivation and reasoning behind this appraoch is sound and agree that we should do it but it is, nonetheless, counter intuative. We're all so used to npm packages being compiled down to something we don't need to worry about; we have all been blithly ignoring node_modules for so long it's become a given, we have coding muscle memory for 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.

Could we also add examples for tooling such as storybook and jest?

yeah great idea, you ok to raise a pr?

we have all been blithly ignoring node_modules for so long it's become a given, we have coding muscle memory for it

this was brought up in infra meetings actually, and we agreed we should really be transpiling everything, beucase you have no control over what's published by other people. but it's not formalised here – it probably should be

Copy link

@JamieB-gu JamieB-gu left a comment

Choose a reason for hiding this comment

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

Looks very sensible!


### Use `@guardian`

- Publish to NPM using the [`@guardian`](https://www.npmjs.com/org/guardian) scope

Choose a reason for hiding this comment

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

Have we decided that npm is the place to standardise, and we should migrate away from any attempts to use GitHub URLs or GitHub Packages?

If so, do we have any more specific recommendations for how to publish to npm? E.g. using np, publishing from main etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we decided that npm is the place to standardise, and we should migrate away from any attempts to use GitHub URLs or GitHub Packages?

I'm not convinced on the value vs confusion this would cause? I'd rather the consistency and the effort to switch all to github or github packages - does it seem worth it?

Copy link
Member

Choose a reason for hiding this comment

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

If so, do we have any more specific recommendations for how to publish to npm? E.g. using np, publishing from main etc.?

Oh, I just noticed that in moving to a separate file we lost a recommendation about using np to publish. @sndrs I'm not sure if that was intentional? We used to have this:

Use [np](https://www.npmjs.com/package/np) to publish packages. Add it as a project dependency and call it via a `release` script in your `package.json`.

Copy link

@JamieB-gu JamieB-gu Mar 2, 2021

Choose a reason for hiding this comment

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

I'd rather the consistency and the effort to switch all to github or github packages - does it seem worth it?

On GitHub dependencies, this means using the github: prefix and #<commit-ish> to specify a repo and branch/commit/tag. This comes with advantages (GitHub release and tagging are all that's needed to publish, can switch between branches and commits easily for testing, etc.) and disadvantages (not straightforward to publish built assets so build on install via prepare etc.). We're using it on AR for a few things - other projects like DCR would have to decide if that would work for them as well.

On GitHub packages, perhaps we should do another spike to see if they're easier to use since @oliverlloyd last looked into them? AFAIK there are some nice upsides (GitHub auth so no need for a separate npm account, ability to publish via GitHub Actions, etc.), but some awkward downsides (auth required to install packages?).

Copy link
Member Author

@sndrs sndrs Mar 9, 2021

Choose a reason for hiding this comment

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

Oh, I just noticed that in moving to a separate file we lost a recommendation about using np to publish. @sndrs I'm not sure if that was intentional?

no an accident, will restore it, thanks @tjmw

Copy link
Member Author

@sndrs sndrs Mar 9, 2021

Choose a reason for hiding this comment

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

on github vs npm, i'm going to move the thread to infra discussions if that's ok? it's an interesting one and could be useful to open up

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.

5 participants