FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Bring life cycle in line with npm standards by johnhaley81 · Pull Request #1055 · nodegit/nodegit · GitHub

  1. nodegit
  2. nodegit

Repository navigation

Bring life cycle in line with npm standards - #1055

Merged
johnhaley81 merged 8 commits into
masterfrom
lifecycle-fixes
Jun 23, 2016
ConversationCommits (8)ChecksFiles changed
Merged

Bring life cycle in line with npm standards#1055
johnhaley81 merged 8 commits into
masterfrom
lifecycle-fixes

Conversation

johnhaley81 commented Jun 14, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator

We were doing our own highly custom (read: fragile) life cycle scripting for installing NodeGit.

This should make it more in line with npm standards.

Bring life cycle in line with npm standards
03d3c33 
We were doing our own highly custom (read: fragile) life cycle scripting for installing NodeGit.

This should make it more in line with npm standards
Update to latest lodash
johnhaley81 force-pushed the lifecycle-fixes branch 2 times, most recently from b0f57f5 to c9c8cf8 Compare June 14, 2016 22:01
Handle legacy npm in lifecycle scripts
9543011 
Also adds console output to let the user know a bit more 
about what's currently being done during install.
johnhaley81 force-pushed the lifecycle-fixes branch 4 times, most recently from 0645b33 to 2e1ac3f Compare June 15, 2016 16:10
tbranyen mentioned this pull request Jun 15, 2016
Closed
valscion mentioned this pull request Jun 20, 2016
Closed
johnhaley81 force-pushed the lifecycle-fixes branch 4 times, most recently from 804d2ab to 39d58e9 Compare June 22, 2016 19:25

johnhaley81 commented Jun 22, 2016

Copy link
Copy Markdown
Collaborator Author

@tbranyen @maxkorp we're passing CI. Can you guys give some 👀 on this?

tbranyen commented Jun 22, 2016

Copy link
Copy Markdown
Member

I'll test it tonight 😍

maxkorp commented Jun 23, 2016

Copy link
Copy Markdown
Collaborator

Talked with john about this in person a moment ago, but putting here for records.

We can't clean the source afterwards.

The standard workflow for electron/nwjs is to npm install normally, then run a rebuild with the appropriate headers. This works with all other native modules i've seen, and currently works with nodegit, but cleaning the src/includes out would break that. We have to be able to run npm run rebuild after install has finished.

Otherwise, gonna review it up right now.

maxkorp reviewed Jun 23, 2016
View reviewed changes
Comment thread .npmignore
/examples/
/generate/
/guides/
/lib/

maxkorp Jun 23, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why ignore lib? all of our js is in there?

johnhaley81 Jun 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lib should already be 100% built and in dist.

maxkorp Jun 23, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That makes complete and total sense and I should have realized that, but sometimes I am do the forget.

maxkorp commented Jun 23, 2016

Copy link
Copy Markdown
Collaborator

This should be good now :)

maxkorp reviewed Jun 23, 2016
View reviewed changes
Comment thread package.json
"lodash": "~3.10.1",
"mocha": "~2.3.4",
"nan": "^2.2.0",
"node-gyp": "~3.0.3",

maxkorp Jun 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The gyp exclusions make me SO GODDAMNED HAPPY.

tbranyen commented Jun 23, 2016

Copy link
Copy Markdown
Member

I'm so far removed from the lifecycle hooks code lately, but this looks v. good, nice work!

maxkorp commented Jun 23, 2016

Copy link
Copy Markdown
Collaborator

@tbranyen There are a lot of improvements in npm3, but there are some definite regressions, and it doesn't fix the fact that npm2 is bundled with node 4 which is LTS :/

By excluding having to generate the code in our bundles, we just have to configure libssh2 if needed, ensure our deps are all installed when we run node-pre-gyp (this is not guaranteed properly in npm2, but it is in npm3 and trying to insure it ourself anyways actually breaks in npm3). We fall back to the default node-gyp now, since all that confusion was from A) node 0.8 and then came back in iojs 1/2.

A lot has changed, but overall the process is easier to follow now in npm3.

johnhaley81 merged commit 5f0fe1c into master Jun 23, 2016
johnhaley81 deleted the lifecycle-fixes branch June 23, 2016 18:59

tbranyen commented Jun 23, 2016

Copy link
Copy Markdown
Member

🎉

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL