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

Update plist parser (and fix package-lock) #1486

Merged
merged 3 commits into from
Oct 5, 2018
Merged

Update plist parser (and fix package-lock) #1486

merged 3 commits into from
Oct 5, 2018

Conversation

mathiasvr
Copy link
Contributor

@mathiasvr mathiasvr commented Sep 27, 2018

Tested on macOS 10.11.6, producing identical Info.plist when packaging.

This PR is blocking PR #1484.

@mathiasvr
Copy link
Contributor Author

This PR also adds package-lock.json and changes the depcheck test script.

Here's the story:

  • We didn't use to have a package-lock.json, but one was included in an old PR, and at the time the CI used npm install and passed.
  • The PR was recently merged, but unfortunately this means that package.json and package-lock.json on the master branch is out of sync.
  • Now, when a package-lock.json file is present, the CI uses npm ci (a variant of npm install) which automatically fails if the lockfile is out of sync.
  • At the same time, the depcheck tool for some reason thinks standard is unused when installed with npm ci instead of npm install
  • To avoid failing the CI tests, this PR includes a proper package-lock.json and updates depcheck to ignore standard

...and that's the reason for the changes. 😄

As an alternative, this could have been fixed by removing the package-lock.json file.

@Borewit
Copy link
Member

Borewit commented Sep 28, 2018

What about using yarn instead (and adding a yarn.lock)?

If you agree, I will open new PR for that.

First fix what we have.

@Borewit Borewit self-requested a review September 28, 2018 09:13
Copy link
Member

@Borewit Borewit left a comment

Choose a reason for hiding this comment

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

LGTM

@mathiasvr
Copy link
Contributor Author

@Borewit What are the benefits of switching from npm to yarn for this project?

@Borewit
Copy link
Member

Borewit commented Sep 28, 2018

What are the benefits of switching from npm to yarn for this project?

I am big opponent from every NPM module which require to be installed globally. You need a very justification to break out of your module scope to be installed globally, and need be non intrusive towards any other module. I noticed that yarn is close to be part of the from the core node & npm distributions. Yarn is probably, which has a good reason to be part of your global "toolbox".

Why?

1.) As developer I experience yarn more reliable in installing / updating the dependencies after running yarn install, as a bonus it I believe it is faster. We all know we run into certain point where we have to delete node_module, my impression is that with yarn is more strict in translating the dependency requirements into installing required dependency, and is much more unlikely you run into that situation (but only if use yarn solely to take care of the dependencies). Jumping from one branch to another, this can be huge benefit, which we do on this project with PR's coming from all directions.

2.) Yarn is more strict in the interpretation of version requirements, therefor it is less likely your code breaks due to some update which was not there before. Which improves configuration management control.

Draw-backs?

Sure, different character. Due to strict interpretation of the version, you need to pursue update versions more often, it doesn't automatically update package.json with patched version after an install.

Give it try yourself, you will have to get used to the command arguments which are a bit different.

@mathiasvr
Copy link
Contributor Author

I think you should create an issue, or maybe a PR if you're up for it, where we can discuss this and get some feedback.
I haven't used yarn that much, but in my experience you also get all of these features with the newest version of npm and a lockfile, maybe except for fast installs.

@sibiraj-s
Copy link
Contributor

Adding to @Borewit .

  1. Yarn gives more reasonable logs than npm.
  2. Definitely cleaner UI in terminal than npm
  3. Yarn handles poor internet connections well.
  4. and more like (workspaces, which is not needed here. plus extra)

The One thing that npm has and yarn doesn't is audit. also it is on its way. yarnpkg/yarn#6409

I think you should create an issue, or maybe a PR if you're up for it, where we can discuss this and get some feedback.

As said can be discussed on new Issue/PR

@mathiasvr mathiasvr changed the title Update plist parser Update plist parser (and fix package-lock) Sep 30, 2018
@Borewit
Copy link
Member

Borewit commented Oct 5, 2018

@feross Can you please consider lowering the number of approvals back to 1? It takes way to long to get PRs approved.

Merged by piggybacking on PR #1042.

@Borewit Borewit merged commit 4ed4d82 into master Oct 5, 2018
@mathiasvr mathiasvr deleted the update-plist branch October 6, 2018 14:11
@lock lock bot locked as resolved and limited conversation to collaborators Jan 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants