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

Improve experience of checking in node_modules #4

Closed
damccorm opened this issue Aug 1, 2019 · 16 comments
Closed

Improve experience of checking in node_modules #4

damccorm opened this issue Aug 1, 2019 · 16 comments
Assignees

Comments

@damccorm
Copy link
Contributor

damccorm commented Aug 1, 2019

Right now, we use husky but its kinda painful. In particular, for users who use editors like VSCode, not having node_modules in the gitignore causes some problems.

We've thought about the idea of having master (or another branch) basically be a dev branch, with no node_modules checked in and then creating an action to automatically create release branches. Definitely open to suggestions here as well.

@shawnnapora
Copy link
Collaborator

What would the dev inner loop look like for this when exploring context-related things? Right now to quickly and iteratively work on this I would get my workflow.yml and actions.yml set up to use my repo with a trigger on PR, and then quickly edit yml/ts files as needed in order to iterate quickly on things I can't test locally.

@Embraser01
Copy link

A solution could be to use the Yarn Zero-Install feature in Yarn v2! Even if it's not released yet, it's quite stable.

With it, dependencies are stored in a single folder as zipfiles, no need to keep a node_modules folder 😃 and any commit is ready to be run.

Drawbacks:

  • Editor compatibility (VSCode has a plugin, but Webstorm need node_modules 🙁)
  • Forced to use Yarn over NPM

I could make a PR if you want to give it a try!

@damccorm
Copy link
Contributor Author

I think what we've settled on (and this is reflected in the current state of this repo/linked docs) is not adding node_modules to master and only installing node_modules on release branches. This also encourages good patterns of having a release branch to pin to.

I think the editor concerns you listed are significant enough, along with it still being in preview, that I probably want to stick with that solution (and I think it works pretty well so I don't see a huge need to switch). With that said, the zero-install feature looks really cool and I'm definitely intrigued - I definitely want to try it out.

Also note, I want to leave this issue open until we update our first party actions to use the pattern I just laid out, so I think it should stay open for now.

@damccorm damccorm self-assigned this Aug 12, 2019
@ajafff
Copy link

ajafff commented Aug 13, 2019

Some thoughts:

  • Does this work for native modules? You will get a different binary depending on your local environment (OS, Node.js version, processor, ...). These aren't guaranteed to work in the target environment. Does this mean native modules can only be used in Docker actions?
  • Without an npm install you will never get new patch releases of dependencies unless you publish a new version of your action
  • checking in node_modules is only part of the story: when using a transpiled language (such as TypeScript) you need to push the transpiled files as well. Typically these are listed in .gitignore.

@damccorm
Copy link
Contributor Author

Does this work for native modules?

Not super strong in this area, but given what you've described I guess not. With that said, it seems like the right choice - I don't really think we want to be building those at runtime. In general, that's the pattern we're trying to avoid - having every action in your workflow build at runtime and adding a bunch of time to every job.

Without an npm install you will never get new patch releases of dependencies unless you publish a new version of your action

Yep, this is by design. A released version should remain static for security reasons (e.g. if someone were to maliciously update a dependency, we don't slide without an explicit choice)

checking in node_modules is only part of the story: when using a transpiled language (such as TypeScript) you need to push the transpiled files as well. Typically these are listed in .gitignore.

Yep. We don't have those in our .gitignore, and that's part of our guidance docs.

@moltar
Copy link

moltar commented Aug 21, 2019

In general, that's the pattern we're trying to avoid

I don't think you can avoid that, especially if you want for actions to work across different OSes.

Native modules won't work even on the same OS across different versions sometimes, or different versions of dependencies (e.g. header files) not to mention an entirely different OS.

One modules comes to mind that often needs rebuilding: node-sass. It seems like every time I do brew update or macOS updates all of my repos need local rebuilds.

Maybe packaging actions as Docker images would work better. But then they won't be able to run natively on an OS, but I am not sure if this is a requirement.

@damccorm
Copy link
Contributor Author

I think in general, my guidance would be to use a docker action if you need native modules. If that absolutely doesn't work for you, you could install your own dependencies at runtime (e.g. exec an npm install from node, or exec a script that does that) - that's not a pattern we recommend though simply because of the time that adds on to execution.

@warrenbuckley
Copy link

@damccorm have you sen this repo/action to help automate this - https://github.com/bumfo/action-deploy

Perhaps something to consider for template use?

@radu-matei
Copy link

How about using the package registry to distribute them?
This way, you don't have to push node_module, and could potentially handle OS-specific dependencies (same way they work right now, if I pull from OS/Arch, I get the correct deps).

@chunlea
Copy link

chunlea commented Aug 23, 2019

Why not we use some tools like gulp or webpack to build the dist, aka lib, and copy the code we need from node_module, rather than checkin the production node_module?

@damccorm
Copy link
Contributor Author

damccorm commented Aug 26, 2019

How about using the package registry to distribute them?
Why not we use some tools like gulp or webpack

We're actually reconsidering this and thinking about both of these/similar solutions. Note that using the GitHub Package Registry isn't supported (yet), but you can use something like webpack currently.

@swinton
Copy link

swinton commented Aug 26, 2019

but you can use something like webpack currently.

I've found zeit/ncc works well.

  1. npm install -D @zeit/ncc
  2. Add a build script to your package.json: ncc build index.js
  3. Update your action.yml's main: './dist/index.js'
  4. npm run build and commit the ./dist/index.js file

@alex-page
Copy link

Thanks @swinton my projects package.json scripts looked like:

    "watch": "ncc build index.js --watch --minify",
    "build": "ncc build index.js --minify",

@thomaseizinger
Copy link

Have you considered requiring actions to attach specific files to a release?
The CI configuration of an action could build an executable file that is attached to the release and downloaded by the runner.
If that is not available, it would fallback to downloading the repositories snapshot.

@bryanmacfarlane
Copy link
Member

Update:

  1. We moved away from husky and updated the docs on how to create a coding branch without node_modules and a distribution branch with prod modules. The bare necessities to run an action from the github graph.
  2. We have future work planned to integrate with GPR as a distribution mechanism (so repo has no branch with prod modules). We evaluated releases (and tar gzs attached) but opt'ed for the better immutability benefits of a gh sha or a GPR type.

Just know that the feedback is heard loud and clear and we're investing in this area.

I'm going to close since we're tracking the GPR work.

Thanks so much for the feedback and the time to write this up.

@gr2m
Copy link

gr2m commented Sep 4, 2019

We have future work planned to integrate with GPR as a distribution mechanism

That is awesome 🚀

0x4007 pushed a commit to 0x4007/typescript-action that referenced this issue Sep 30, 2024
0x4007 pushed a commit to 0x4007/typescript-action that referenced this issue Sep 30, 2024
0x4007 pushed a commit to 0x4007/typescript-action that referenced this issue Oct 1, 2024
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

No branches or pull requests