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

Use one package manager, not two #11278

Closed
dreamofabear opened this issue Sep 12, 2017 · 12 comments · Fixed by #11332
Closed

Use one package manager, not two #11278

dreamofabear opened this issue Sep 12, 2017 · 12 comments · Fixed by #11332

Comments

@dreamofabear
Copy link

Now that NPM supports version locking in v5, it would be nice to only have a single package manager, e.g. replace yarn.lock with package-lock.json.

/to @danielrozenberg /cc @rsimha-amp @erwinmombay

@rsimha
Copy link
Contributor

rsimha commented Sep 13, 2017

See #11289 for a discussion + experiment on caching the results of npm install.

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017

@danielrozenberg @choumx @erwinmombay Ever since we moved to package-lock.json, we're running into the case where some of our dependencies have different sets of sub-dependencies on linux and macos, due to which there's no stable state for package-lock.json. I wonder if there's a way to fix this, by always installing all optional dependencies, irrespective of the platform.

@rsimha rsimha reopened this Sep 19, 2017
@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017

This is an issue on npm: npm/npm#18135

I wonder if we should move back to using yarn until this is fixed.

@danielrozenberg
Copy link
Member

Do we have to include the package-lock.json file? It's recommended practice but if it's buggy then the recommendation is moot. We can add it to .gitignore until npm/npm#18135 is fixed

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017

It turns out that package-lock.json is ignored on Travis if there's a cached node_modules directory. So right now, we aren't actually using the lock file anywhere. See travis-ci/travis-ci#7936

I like the idea of adding package-lock.json to .gitignore for now, since I'm not sure that it's doing anything for us. We could also delete it, but I may be missing something.

Remind me again, why are we using a lock file? :/

@danielrozenberg
Copy link
Member

We can think of package-lock.json as "last known working state of the package dependencies"
(StackOverflow.) It's useful to have, but ideally package.json should always be able to generate functioning code

It also locks in the versions of unlisted packages (i.e., the packages that are implied via the dependencies of the packages that are declared explicitly) - again, for the purpose of locking in a last known good version

@dreamofabear
Copy link
Author

Because accidental breaking changes can slip into semantically versioned packages. That means new AMP contributors may get into a state where npm install doesn't work out of the box. This risk increases with the total number of dependencies, direct and indirect. More reading: https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all/

Between the Travis and NPM issues you linked, maybe we should move back to yarn. 😄

@dreamofabear
Copy link
Author

For some context, the original FR from over a year ago: #2771

@erwinmombay
Copy link
Member

erwinmombay commented Sep 19, 2017

i would bring back yarn for security and other caches having deterministic builds (even more important than having working local builds for amp contributors)

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017

Gotcha. I agree that moving back to yarn (lock file, caching, presubmit check to make sure lock file is updated, etc.) is the way to go for now.

@danielrozenberg
Copy link
Member

I'm on it

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017

For still more context, see npm/npm#17722. I don't think there's a supported way to generate a steady-state version of package-lock.json from mac and linux.

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

Successfully merging a pull request may close this issue.

4 participants