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

Implement github actions #29

Closed

Conversation

jamescurtin
Copy link
Contributor

Implements CI using Github Actions.

Can be useful for enforcing that tests/linting passes before merging in PRs.

Can also be used to automate the process of publishing new releases to NPM.

@jamescurtin
Copy link
Contributor Author

For reference, here is the passing CI pipeline for this PR.

@fabio-looker
Copy link
Contributor

fabio-looker commented Dec 10, 2019

Hey James!

This looks great, can't wait to get it merged in. I think I may have set up my package & package lock files in a hacky way and would love if we could maybe sync on the best way to do it before going forward with this change...

Previously, I noticed that whenever I would npm install -g look-at-me-sideways, I would always get all the dev dependencies installed because they were in npm-shrinkwrap.json. So, as of v1.1.1, I took the workaround here, and essentially published the prod npm-shrinkwrap.json and maintained a separate npm-shrinkwrap.dev.json

I see your PR has an npm-shrinkwrap.json with all the dev dependencies too, and am wondering whether this might re-introduce the behavior where prod installs pull down all the dev dependencies?

Currently, my recommendation for dev work is to simply copy the dev package lock file over the prod one before npm installing, but I think there might be a better solution leveraging both package-lock.json and npm-shrinkwrap.json with one of them in .npmignore. (For reference, per https://docs.npmjs.com/files/shrinkwrap.json , if you use both package-lock.json and npm-shrinkwrap.json, the latter takes precedence. However, NPM docs are not very helpful in explaining how to compose these for the CLI tool use case...) Not sure if you have any other ideas?

@jamescurtin
Copy link
Contributor Author

Based on my understanding of the shrinkwrap file, the installation of all devDeps by default is the intended behavior of the utility:

From the docs (emphasis added):

The recommended use-case for npm-shrinkwrap.json is applications deployed through the publishing process on the registry: for example, daemons and command-line tools intended as global installs or devDependencies. It’s strongly discouraged for library authors to publish this file, since that would prevent end users from having control over transitive dependency updates.

So it seems the shrinkwrap is best suited for use cases where you intend to proscriptively pin the devDeps for the user (which I don't think is what's intended).

Going to push a commit that switches to a standard package-lock.json, which I think should solve the issues discussed above. Let me know if you'd prefer something different or if I missed anything!

@fabio-looker
Copy link
Contributor

I believe that this npm doc is making a subtle and non-explicit distinction between library packages and end-user executable packages, and that the highlighted sentence is specifically for the former, while LAMS is the latter.

Other hints at this distinction are:
(1) the mention of transitive dependencies in that sentence, which would only be relevant if LAMS were not the top-level install.
(2) The heading - "a publishable lock file"
(3) "The recommended use-case for npm-shrinkwrap.json is applications deployed through the publishing process on the registry: for example, daemons and command-line tools intended as global installs..."

Because of the above, I do think npm-shrinkwrap.json with prod dependencies is the NPM-intended way of publishing a CLI tool to NPM. However, what I'm not sure of is what the corresponding approach for locking dev dependencies for development and CI testing would be :-/

@fabio-looker
Copy link
Contributor

PS. I will try to set aside some time in the next couple of days to test out what happens, both with this approach and some other alternatives

@fabio-looker
Copy link
Contributor

Hey James! Sorry for the radio silence on my end. Holidays! Hope yours were good :)

Anyway, I'm trying to get back up to speed, but I don't see the actions in this PR or your branch anymore - did you change something, or am I just blanking...?

@fabio-looker
Copy link
Contributor

Nevermind, I found it! Sorry about that.

I got the changes incorporated here, with a couple minor tweaks: a361a68

Thanks for getting me going on this!

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.

2 participants