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

Simplify build process and published packages #4

Merged
merged 1 commit into from
Oct 2, 2014

Conversation

eush77
Copy link
Contributor

@eush77 eush77 commented Sep 29, 2014

  1. Remove generated files from the repository.
    This will eliminate the need for synchronizing src/ and lib/ directories by hand and lead to the cleaner git history.
  2. Register "prepublish" and "test" npm scripts.
    These scripts are standard interfaces NPM provides. "Prepublish" will fire any npm publish or npm install. Of course xyz is still managing the publishing, but it would be still better if NPM kept lib/ in sync automatically for you.
  3. Discard CoffeeScript sources and build files from published tarballs.
    Leave CoffeeScript for development and publish the compiled JS only. Users don't need any CoffeeScript.
    The package itself becomes tighter:
$ npm pack

> string-format@0.2.1 prepublish .
> make

node_modules/.bin/coffee --compile --output lib -- src/string-format.coffee
string-format-0.2.1.tgz

$ tar tf string-format-0.2.1.tgz
package/package.json
package/.npmignore
package/README.md
package/LICENSE
package/lib/string-format.js

What do you think?

@davidchambers
Copy link
Owner

Thank you for submitting this pull request. Your interest has motivated me to spend time wrapping up #2, on which I began work a year ago!

Remove generated files from the repository [to] eliminate the need for synchronizing src/ and lib/ directories by hand and lead to the cleaner git history.

Thanks to #3 it's no longer necessary to update lib/ by hand. Running make release-patch will run a script to update the JavaScript file. This keeps the history clean, as the JavaScript file is only updated when we publish a release. See davidchambers/CANON@22f3f2d for an example of how this looks in practice.

"Prepublish" will fire any npm publish or npm install. Of course xyz is still managing the publishing, but it would be still better if NPM kept lib/ in sync automatically for you.

As mentioned above, this is handled by the current release process.

Discard CoffeeScript sources and build files from published tarballs.

I'm very much in agreement on this. I'd love to see a standalone pull request which adds an .npmignore.

@eush77
Copy link
Contributor Author

eush77 commented Sep 30, 2014

Thanks to #3 it's no longer necessary to update lib/ by hand.

Why does lib/ needs to be part of the repo in the first place? It is not a source directory. (This also coincides with the “cleaner history” part.)

Auto-committing it on publishing is definitely better than syncing by hand, but still most of the time lib/ and src/ won't be in sync, which is a source of confusion and another problem.

Not in the repo ═ no need for any auto-committing hacks at all. After all, lib/ belongs to the package and that's all it is used for.

As a bonus, it would become possible to replace make setup with far more familiar and standard npm install.

As mentioned above, this is handled by the current release process.

There is no real need for the last lines in scripts/prepublish as it can be easily delegated to NPM. Also registering make test as npm test will convey its purpose through a standard NPM interface.
However, this part of the changes is mostly cosmetic and not so important.

@davidchambers
Copy link
Owner

[M]ost of the time lib/ and src/ won't be in sync, which is a source of confusion and another problem.

Good point.

Not in the repo ═ no need for any auto-committing hacks at all. After all, lib/ belongs to the package and that's all it is used for.

I find this line of reasoning compelling. You're winning me over. :)

Now that I think I'm on board with your proposal I'll review the pull request in detail.

@@ -1 +1,2 @@
/node_modules/
lib/
Copy link
Owner

Choose a reason for hiding this comment

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

Please include a leading slash, to anchor this to the repository root. I realize that in practice it won't make a difference, but /foo/bar/baz/lib/quux.js should not be ignored.

@davidchambers
Copy link
Owner

This is looking good. Would you mind squashing all these changes into a single commit?

@eush77
Copy link
Contributor Author

eush77 commented Oct 1, 2014

Squashed!

@davidchambers
Copy link
Owner

Thanks! Now to make sure I understand all the things that'll happen when I run make release-patch.

  1. LEVEL will be set to patch.
  2. Make will run the following command:
node_modules/.bin/xyz --message X.Y.Z --tag X.Y.Z --repo git@github.com:davidchambers/string-format.git --script scripts/prepublish --increment patch
  1. xyz will prompt for confirmation.
  2. xyz will run the prepublish script, which will update the version string in the source file and add the change to the Git index.
  3. xyz will update the version string in package.json.
  4. xyz will git commit, git tag, and git push. The changes to package.json and src/string-format.coffee will be included in the commit. The JavaScript file is never committed, as /lib/ is ignored in .gitignore.
  5. xyz will npm publish, which will trigger the prepublish hook.
  6. The prepublish hook will invoke make with no arguments, which will create lib/string-format.js.
  7. npm will create a tarball, which will include lib/string-format.js but will not include the source file, the prepublish script, the test file, or the makefile, all of which are ignored in .npmignore.

Is this correct, @eush77?


While we're at it, I suggest we remove the version string from src/string-format.coffee. We can then remove the prepublish script, which will reduce the cognitive load. :)

@eush77
Copy link
Contributor Author

eush77 commented Oct 1, 2014

Yeah, that seems to be the way it will work.


While we're at it, I suggest we remove the version string from src/string-format.coffee.

OR we can include the version from package.json like they do so it will always be up-to-date:

String::format.version = format.version = require('../package.json').version

@davidchambers
Copy link
Owner

That would work in Node, but not in the browser.

Also, there's a universally applicable way to get the version string in Node:

> require('string-format/package.json').version
'0.2.1'

I've convinced myself. Let's remove the version property. Mind making the necessary changes?

@eush77
Copy link
Contributor Author

eush77 commented Oct 1, 2014

Sorry, I forgot to remove /scripts/ from .npmignore initially. I rebased.

@eush77
Copy link
Contributor Author

eush77 commented Oct 1, 2014

Or should I squash it again?

@@ -1 +1,2 @@
/node_modules/
/lib/
Copy link
Owner

Choose a reason for hiding this comment

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

Let's alphabetize these. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@davidchambers
Copy link
Owner

This patch is looking very nice. Yes, please squash once more. I also pointed out one incredibly minor issue you could address at the same time.

@@ -5,6 +5,10 @@
"author": "David Chambers <dc@davidchambers.me>",
"keywords": ["string", "formatting", "language", "util"],
"main": "./lib/string-format",
"scripts": {
"prepublish": "make",
Copy link
Owner

Choose a reason for hiding this comment

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

One last change: let's "make clean && make" so we recreate the JavaScript file even if for some reason its modification time is later than that of the source file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@davidchambers
Copy link
Owner

Lovely work! Merging.

davidchambers added a commit that referenced this pull request Oct 2, 2014
Simplify build process and published packages
@davidchambers davidchambers merged commit dcd6556 into davidchambers:master Oct 2, 2014
@eush77 eush77 deleted the npm-integration branch October 2, 2014 06:16
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.

2 participants