Skip to content

Conversation

@DanielRuf
Copy link
Contributor

Running npm ci or yarn throws an error because patch-package can not be found. Migrating from prepare to postinstall solves this.

@lmccart lmccart merged commit 0956fc9 into processing:master Aug 1, 2018
@DanielRuf DanielRuf deleted the fix/use-postinstall-for-ci branch August 1, 2018 04:29
@Spongman
Copy link
Contributor

Spongman commented Aug 10, 2018

@DanielRuf can you explain the rationale for this change? it looks like postinstall is run when the p5.js npm package is installed, but this is causing an error since patch-package is a devDependency, NOT a dependency, so it's not available when the p5 npm package is installed, only when npm install is run in the p5.js source branch.

the fact that npm install works fine and npm ci does not just tells me there's something broken with npm ci.

also, yarn v1.9.2 works fine for me.

@DanielRuf
Copy link
Contributor Author

In the current docs (master) of patch-package, postinstall is used. Because prepare does not work with a local patch-package when using yarn and throws an error on install.

Either we need this as prepush/commit/publish or Git hook.

@Spongman
Copy link
Contributor

Spongman commented Aug 10, 2018

Err it works for me with yarn just fine. Which version did you try?

@DanielRuf
Copy link
Contributor Author

Err it works for me with yarn just fine. Which version did you try?

The latest, 1.9.4.

@DanielRuf
Copy link
Contributor Author

Also related to npm ci:

ds300/patch-package#46

@DanielRuf
Copy link
Contributor Author

Probably confused something but definitely a problem with npm ci.

@lmccart
Copy link
Member

lmccart commented Aug 10, 2018

Ok, so if I'm understanding correctly it would be ok to switch back to prepare and forgo npm ci for now? It is important that npm install p5 work since many users get the library that way, but is it ok that npm ci won't work then? Is it correct to think that's more of a nice-to-have than a necessity?

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Aug 10, 2018

Then many CI builds will be broken. For example we used and still use npm ci in many projects and so do others. For example if there is a package-lock.json and no explicit install command in a .travis.yml, Travis CI will use npm ci by default and npm ci has many advantages over npm i.

Ok if it is broken, revert it.

At least prepare still does not work with yarn with locally installed packages (we migrated all projects to yarn).

I guess npm itself is broken then?
But still, prepare is not recommended anymore and yarn also fails with prepare.

@Spongman
Copy link
Contributor

Spongman commented Aug 10, 2018

I'm confused, which Travis build will break if we switch this back?

@DanielRuf
Copy link
Contributor Author

The builds which use npm ci and possibly yarn.

@Spongman
Copy link
Contributor

i'm not aware of any travis builds that build the p5.js project that use yarn or npm ci. really the only one that matters is the one that builds the PRs here, and that doesn't use either.

@meiamsome
Copy link
Member

@DanielRuf If I am correct in what I've researched, the errors will only people building p5 directly with npm ci/yarn that will experience that issue, and downstream users of the package would have no issues - is that correct? If so, we should switch back to prepare and ship a new version because we don't use npm ci in our builds (yet - I was going to suggest we switch to that soon)

@lmccart
Copy link
Member

lmccart commented Aug 10, 2018

Ok, I think the # of end users trying to install with npm is probably larger than the number of developers trying to build from source using npm ci or yarn so I think it seems fair to prioritize them in this matter. Hopefully this gets resolved higher up in the near future and we can revisit again later. Thanks all!

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.

4 participants