Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

prepare script not working with yarn #38

Open
sndrs opened this issue Dec 4, 2020 · 11 comments
Open

prepare script not working with yarn #38

sndrs opened this issue Dec 4, 2020 · 11 comments

Comments

@sndrs
Copy link
Member

sndrs commented Dec 4, 2020

I'm not sure if it's yarn-specific, but after updating to v1 in frontend, I'm not getting a dist dir:

image

what made you go with prepare script over publishing an artefact?

@JamieB-gu
Copy link
Contributor

I'm not sure if it's yarn-specific, but after updating to v1 in frontend, I'm not getting a dist dir

Odd, what happens if you run yarn install?

what made you go with prepare script over publishing an artefact?

Apps-rendering and image-rendering consume the package from GitHub - which means they pull the source code. prepare runs when the package is installed, generating the dist directory and TS declarations.

We could also publish an artefact if you think it would make things easier for registry consumers?

@sndrs
Copy link
Member Author

sndrs commented Dec 7, 2020

Odd, what happens if you run yarn install?

it doesn't run the prepare script, either an add or install :(

it does sound like it should though, for sure...

We could also publish an artefact if you think it would make things easier for registry consumers?

I guess this would have the benefit of ensuring that the build was successful before publishing too

@JamieB-gu
Copy link
Contributor

it does sound like it should though, for sure

Based on the second bullet-point in guardian/cdk#9, perhaps @akash1810 knows something about this?

@akash1810
Copy link
Member

Ah this was fun to debug! There appears to be a bug in yarn causing this and the proposed fix is scary after reading this.

The changes in guardian/cdk#9 worked and I'd recommend applying the same changes here.

It's worth noting that for the cdk project, we've since started publishing to npm after witnessing some strange behaviour in CI for dependant projects. That is, I'd, more strongly, recommend publishing to npm and would be interested to understand the use-case for installing from git.

cc @jamie-lynch @stephengeller who travelled this journey with me and might remember more.

@JamieB-gu
Copy link
Contributor

Thanks Akash!

witnessing some strange behaviour in CI for dependant projects

Interesting, what sort of strange behaviour? We've got a handful of projects using GH installs 🤔... it's not related to Docker containers in TC by any chance?

That is, I'd, more strongly, recommend publishing to npm

This repo is published to npm as well, it supports both registry installs and git installs! 🎉

@akash1810
Copy link
Member

akash1810 commented Dec 10, 2020

what sort of strange behaviour?

A general failure to install dependencies, with an unclear reasoning/stack trace, see the oldest entries here. There were also some failures on another project that's built in TeamCity which I'm unable to find the link to, sadly.

This repo is published to npm as well, it supports both registry installs and git installs! 🎉

Woop! It looks like the published library only includes the original TypeScript and not the compiled JS and declaration files? That is, there's still a reliance on prepare running when installing from a registry?

To test:

docker run -it --rm --name test node:14.15.1 /bin/bash -c "npm install @guardian/types; ls -lah node_modules/@guardian/types"

or

docker run -it --rm --name test node:14.15.1 /bin/bash -c "yarn add @guardian/types; ls -lah node_modules/@guardian/types"

@JamieB-gu
Copy link
Contributor

JamieB-gu commented Dec 10, 2020

It looks like the published library only includes the original TypeScript and not the compiled JS and declaration files?

Ah, yeah good point. We switched over to generating declaration files quite recently (#35), and I think the only project that uses that version is Apps-Rendering, which installs via GitHub. I believe @sndrs is planning to publish a build artifact to the registry to remedy this? So yeah, currently there is still this dependency on prepare for registry installs, although since that's working fine in AR (via GitHub) I would expect it to work ok for registry installs too 🤔.

@sndrs
Copy link
Member Author

sndrs commented Dec 11, 2020

I'm really not clear what the benefit is of not publishing a build artefact? do you get something platform-specific by building each time it installs?

one definite benefit of publishing the artefact (along with source code, if that's useful) is that you can fail the release process if the build fails.

this is only true if building is idempotent of course...

@JamieB-gu
Copy link
Contributor

I'm really not clear what the benefit is of not publishing a build artefact? do you get something platform-specific by building each time it installs?

Oh I don't think there are any benefits to not doing it. But I don't think a build artifact is relevant for GitHub installs? Where would it be hosted? The prepare script exists here to build the assets from source when the source is pulled from GitHub (BSD-style).

@akash1810
Copy link
Member

akash1810 commented Dec 11, 2020

Where would it be hosted?

Within a GH release? For example https://github.com/guardian/cdk/releases/tag/v0.3.0 (np does this for free!).

In the guardian/cdk example, if you install from GH at #v0.3.0, you'll use the tarball on that tag.

@JamieB-gu
Copy link
Contributor

you'll use the tarball on that tag.

But that's a tarball of the source isn't it, not the built artifact? How would GitHub know how to build and package your code when you create a release?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants