davidchambers / tutor

JavaScript interface for the Gatherer card database
https://gatherer.wizards.com/
Do What The F*ck You Want To Public License
149 stars 18 forks source link

add `make release` as prepublish script #83

Closed nicknovitski closed 10 years ago

nicknovitski commented 10 years ago

I was looking through the package.json documentation again, and noticed some repeated (advice on using scripts.prepublish to do coffee->js processing)[https://npmjs.org/doc/misc/npm-scripts.html]. What do you think?

nicknovitski commented 10 years ago

Ohoho, silly me, make release does more than just rebuilding those files.

nicknovitski commented 10 years ago

prepublish is run 1) before npm publish happens, which is once every release 2) after npm install happens locally, which happens all the goshdarn time

Whereas make release tagging a commit and the like should clearly only happen on (1). If all that were to be automated, it would be a publish or postpublish script, but then it would have to get the version from the package.json, and I suppose error if that version had been published previously? Either way, it's a separate feature.

davidchambers commented 10 years ago

Aside from removing the need to commit changes to the JavaScript files (which sounds great), will this affect the development workflow in any way? Will make test continue to test the source files?

nicknovitski commented 10 years ago

make test works: the test file requires directly from the src directory, which should also mean that make test doesn't actually depend on make all.

I'm still trying to find a way to verify to my satisfaction that it installs right. You can't npm install a git url with a branch, it seems.

nicknovitski commented 10 years ago

This seems very odd, but when I try to install the package from my working directory, even though the lib directory has been built, it is not copied to the node_modules directory along with all the other files, so require('tutor') fails.

I can fix that by adding a files array to package.json, but then you need make and maintain a list of every file you want included, which could be an ongoing pain. I would rather first understand how a directory could both be and not be there. I thought npm installed things locally from directories with package.json files, but maybe it's also detecting and leveraging git ls-files somehow?

davidchambers commented 10 years ago

I'm in favour of this change, but I agree that the first step is to gain a good understanding of how it works. :)

nicknovitski commented 10 years ago

This thread has squatted shamefully at the bottom of my inbox for the past three months, and now, with 30 minutes thought...

Running npm install in a local directory only ever installs that packages dependencies. To install the package itself, there is npm link. Since that also runs the prepublish script, it's essentially simulating a publish-and-then-download, and it works just fine.

A downside I now see is that it would break installing the package directly from the git repository.

davidchambers commented 10 years ago

Are there any project using this approach? It seems good in theory, but I haven't thought through all the ramifications. I'm happy to defer to you on this one, @nicknovitski. We can always reinstate the JavaScript files if we decide it was a mistake to remove them.

nicknovitski commented 10 years ago

Project which does this: assaf/zombie.

BUT: I also found a few projects that just add a "dist" or "publish" command to their makefile which builds and then calls npm publish. And, I now notice, we've got a "release" command that does except that last bit...

So we can indeed hide the built javascript, but we don't actually need a prepublish script! I'm going to change this commit to just be removing lib and adding to the .*ignore files.