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

Either commit package-lock.json, or gitignore it #863

Closed
alexweissman opened this issue Mar 24, 2018 · 6 comments
Closed

Either commit package-lock.json, or gitignore it #863

alexweissman opened this issue Mar 24, 2018 · 6 comments
Assignees
Labels
assets Related to the assets feature developer experience The one building with UF needs discussion A decision needs to be taken by the dev team
Milestone

Comments

@alexweissman
Copy link
Member

Right now, build/package-lock.json is created for the first time when you run build-assets or bake. We should decide if we want to commit it to UF (thus locking devs at the same versions of the packages that we used in the release), or gitignore it.

I'd argue the same goes for composer.lock.

@alexweissman alexweissman added assets Related to the assets feature developer experience The one building with UF labels Mar 24, 2018
@alexweissman alexweissman added this to the 4.2.0 milestone Mar 24, 2018
@lcharette
Copy link
Member

@Silic0nS0ldier This file is annoying me. What should we do with it ?

@Silic0nS0ldier
Copy link
Member

From a security perspective, I vote keep it. GitHub routinely runs a security audit on files like this, and if we remove it we risk not finding out about a known vulnerability. Vulnerable dependencies could inject code that is run client side (or even when the project is deployed) permitting session hijacking attacks and at worse server takeover. Node isn't the most locked down system by default, so keeping tabs on vulnerabilities that affect us is important.

My opinion regarding the file might change if alternatives turn up. (an alternative will be needed if user extensions get hooked up in the future)

@lcharette
Copy link
Member

Above the security audit, thing is, just like composer.lock, that file will change when dev add their own dependencies. Since a dev shouldn't commit his code as part of the UF main repo, that file will stay in a modified state forever, which cause problem when switching branch or tags as I'm experiencing right now.

One of composer.lock goal is to install the same same dependencies version as the staging env when deploying to a live site using composer install. Only the update command will update the lock file with newer version. This is something we don't actually use in UF, and for the package lock file, this is not used at all, since the Bakery command always seams to run the "update" command.

Thing is, if it doesn't bring any benefit right now, other than the security audit thing, I vote we gitignore it. But at the same time, I vote we do look for a way for a dev to be able to commit this file with all their project dependencies somehow. Probably something we can work out along #830.

@lcharette lcharette added the needs discussion A decision needs to be taken by the dev team label Oct 21, 2018
@Silic0nS0ldier
Copy link
Member

Considering how UserFrosting is used, I'm thinking ignoring it is the better choice. Will just annoy users with merge conflicts, etc. Come V5 this shouldn't be an issue (🤞).

@lcharette
Copy link
Member

@Silic0nS0ldier
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assets Related to the assets feature developer experience The one building with UF needs discussion A decision needs to be taken by the dev team
Projects
None yet
Development

No branches or pull requests

3 participants