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

Remove package-lock.json #1375

Merged
merged 2 commits into from
Apr 12, 2018
Merged

Remove package-lock.json #1375

merged 2 commits into from
Apr 12, 2018

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Apr 4, 2018

We don't have any dependencies so this isn't as important to us, it's also ugly
to keep seeing these in PRs.

We don't have any dependencies so this isn't as important to us, it's also ugly
to keep seeing these in PRs.
@Tyriar Tyriar added this to the 3.4.0 milestone Apr 4, 2018
@Tyriar Tyriar self-assigned this Apr 4, 2018
@amejia1
Copy link
Contributor

amejia1 commented Apr 6, 2018

The package-lock.json file also handles dev dependencies. It would be great if anyone developing xterm.js had the same dev dependencies installed, such as tslint. This also affects the CI builds.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 6, 2018

@amejia1 seems like a lot of hassle/noise maintaining this file to fix a problem that's never come up in the development of the library before. I'd prefer to pin the devDependencies to specific versions than keep this, but still that's overkill for such a relatively simple build process imo.

@mofux
Copy link
Contributor

mofux commented Apr 6, 2018

Let's get rid of it. It only causes confusion and I've never seen a problem without it. IMHO the only case where this file is useful is to record and restore a dependency state for a software release. But since we don't have any dependencies in our releases we can skip this file.

@amejia1
Copy link
Contributor

amejia1 commented Apr 7, 2018

@Tyriar The package-lock.json file covers pinning the entire (dev)dependency tree as well (dependency's dependencies and so on). @mofux It is indeed useful for reproducing the dependency tree for other devs and for the CI builds.

Maybe development of xterm.js has worked fine without it before, but your dependencies are still being updated. The proposed use of prebuild and prebuild-install in node-pty may have an impact now or in some future release of those two packages. The '--include' option in prebuild to include other file types (exe, dll, so on) was only recently added for example. That's just one example. I think explicitly defining and using the dependency tree for the sake of avoiding the "works on my dev environment" issues outweighs the inconvenience of seeing it in PRs.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 11, 2018

The package-lock.json file covers pinning the entire (dev)dependency tree as well (dependency's dependencies and so on)

@amejia1 oh I understand that, but since we're not actually shipping any dependencies this could only affect us for dev dependencies. And I would argue that the chance it affects us at all is not worth the maintenance burden of keeping the package-lock. If we did ship dependencies I would advocate for the opposite.

I think explicitly defining and using the dependency tree for the sake of avoiding the "works on my dev environment" issues outweighs the inconvenience of seeing it in PRs.

AFAIK we have never had such an issue in the ~2 years I've been involved in the project.


Requesting more feedback from @xtermjs/core

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 let's get rid of this. package-lock.json has never been useful for this particular project or my personal work.

If it turns out people have hard time developing xterm.js, we can search for a solution at that time. Also, even in that case I am pretty confident that Yarn will make the cut instead.

@Tyriar Tyriar merged commit da8151b into xtermjs:master Apr 12, 2018
@Tyriar Tyriar deleted the remove-package-lock branch April 12, 2018 17:03
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