Node.js, Browserify & UMD support

[jesseditson]

Addresses #16, making this module compatible with node.js & browserify builds:

Changeset:

• add package.json with build deps and server-side deps
• move to commonjs dependencies, don't expose globals
• change FileAPIReader to be a property of ID3 instead of global (breaking)
• Makefile instead of bash script for compiling dist
• switches closure compiler for browserify & uglify, with --standalone (for UMD compatibility)
• add ultra-dumb test to just smoke test server side stuff (run with npm test) (small progress towards Create test suite #22)
• update README with node.js & UMD usage instructions
• bump minor version since we're introducing a breaking change (FileAPIReader access)
• debug version (by running make dev) with sourcemaps and no obfuscation

Caveats:

• I didn't publish this to npm, because I figured you should own that stuff. In the meantime, it can be installed by running: npm install --save jesseditson/JavaScript-ID3-Reader#module_loaders
• This introduces a breaking change which is that FileAPIReader is no longer a global. I updated the examples and made this a minor version bump (I'm assuming you're using semver) to avoid any confusion. Otherwise the API is identical.
• Because I removed closure in favor of uglifyjs & browserify (browserify is incompatible with closure compiler), the resulting dist file is slightly larger. I turned off all the bloat from the browserify builtins, but there's still some overhead from browserify & UMD. The final result is that the packaged version went from ~15k to ~27k. For most people this should be fine, but I wanted to mention it because it's a fairly large difference in terms of %.

If this looks good, you can run npm publish from the root directory after merging this and it will automatically create the id3-reader package under your name. Alternatively, it looks like the current id3 package on npm is abandoned, and was originally derived from this project anyway - so I'm sure if you can get a hold of @tim-smart he'd be happy to make you the author of id3. If that happens you can just change the name field in package.json and run npm publish.

Let me know if there's anything I can fix up on this guy!

Jesse

[tim-smart]

Let me know if you need the author changed

[aadsm]

First of all: Holy shit, this is amazing :O! Thank you so much for doing this!

I've been working on the next version of this library, with a slightly new API and architecture, it includes exhaustive tests, browser and node support just like you have in this pr. I've done 1/3 of it but still need to take some time to finish it up. In the meanwhile I hope we can push this in.

The things that I'm a bit worried with:

• The removal of the global FileAPIReader, can we still make it global and avoid this breaking change? Maybe we can keep the old code and have window["FileAPIReader"] = FileAPIReader when window exists?
• The code size is quite a big change.. almost double size. Here's an idea to keep an (optional) google closure compilation: Create a with-google-closure.js (or whatever) that defines a require function that basically returns the right global object based on the path like: require('./binaryfile.js') returns window.BinaryFile. We should then be able to compile it by including the with-google-closure.js in the closure command. In this scenario we'll still need to wrap the code in a function though, otherwise the var BinaryFile = require(...) would just override the global one. Do you think this might work?

Can you update the license field in package.json to BSD-3-Clause? BSD is not a valid one according to https://spdx.org/licenses/.

Again, really happy with this!

@tim-smart thanks for the offer. Wouldn't this be a breaking change for people already using the current one?

[tim-smart]

I wouldn’t be too worried about breaking changes, especially if people are using semver correctly

On 22/06/2015, at 4:08 pm, António Afonso [email protected] wrote:

First of all: Holy shit, this is amazing :O! Thank you so much for doing this!

I've been working on the next version of this library, with a slightly new API and architecture, it includes exhaustive tests, browser and node support just like you have in this pr. I've done 1/3 of it but still need to take some time to finish it up. In the meanwhile I hope we can push this in.

The things that I'm a bit worried with:

The removal of the global FileAPIReader, can we still make it global and avoid this breaking change? Maybe we can keep the old code and have window["FileAPIReader"] = FileAPIReader when window exists?
The code size is quite a big change.. almost double size. Here's an idea to keep an (optional) google closure compilation: Create a with-google-closure.js (or whatever) that defines a require function that basically returns the right global object based on the path like: require('./binaryfile.js') returns window.BinaryFile. We should then be able to compile it by including the with-google-closure.js in the closure command. In this scenario we'll still need to wrap the code in a function though, otherwise the var BinaryFile = require(...) would just override the global one. Do you think this might work?
Can you update the license field in package.json to BSD-3-Clause? BSD is not a valid one according to https://spdx.org/licenses/ https://spdx.org/licenses/.

Again, really happy with this!

@tim-smart https://github.com/tim-smart thanks for the offer. Wouldn't this be a breaking change for people already using the current one?

—
Reply to this email directly or view it on GitHub #51 (comment).

[jesseditson]

Hey there, added some changes:

• new license
• get compiled size down to 17k*
• expose FileAPIReader
• include compiler.jar
• new make tasks: debug (adds sourcemaps, no obfuscation) and compile (aggressive compilation, way smaller)

*some notes on closure + browserify:

TL:DR; The lib can now be compressed by running make compile, and we'll get everything above (including UMD support) and only add 1 kilobyte.

Details:

At first, I went for the "simple" approach of just adding commonjs compilation to closure compiler, via the experimental flags - however, because I'm doing a conditional require and the dependencies are resolved before dead code is stripped, I couldn't figure out how to trick closure in to ignoring the node.js only code paths. Additionally, switching to closure over browserify entirely means we lose the UMD imports, so this library wouldn't work with AMD or other module loaders, which would be a bit of a shame.

I tried a few alternative approaches as well, but ultimately went down the route of some sed precompilation to get things where they needed to be, namely taking a non-minified browserify file and forcing the exports to assign to string keys by doing a simple substitution. This is a bit of a hack, but it was the cleanest I could get it without giving up more than a kilobyte. Once that was in, the closure compiler can run on the browserify output, and we get the best of both worlds in terms of weight and compatibility.

One last note is that I set the compiled version to be dist/id3-minimized.min.js, the browserify output (from make debug or make dist) is in dist/id3-minimized.js. This naming is a bit confusing though, perhaps I should just always have make spit out id3-minimized.js regardless of settings?

[aadsm]

Oh, interesting, I didn't know Malte added experimental support for CommonJS modules.
I'm pretty happy with sed and compiling the browserify output, a hack but a pretty sweet one.
This PR is getting better by the day :)

I'm fine with always using the same id3-minimized.js name but let's commit the closure version.
Thank you for fixing all the indentation too!

Let me just test this a bit more during the weekend, I've had issues in the past with some minimizers.

Btw, the mp3 file you point to, what is the license? I prefer using a Creative Commons Share Alike (CC BY-SA).

[aadsm]

Just tested and everything looks great. Can you add the compiler.jar to the LICENSE file and a link to the origin of the file?
Also, how about making both minimized versions output to id3-minimized.js?

[aadsm]

I finally had the time to make the changes I wanted and merged this to master, thank you very much for this PR!

@tim-smart any chance I can publish to npm's id3 then?

[jesseditson]

Awesome! Sorry about the radio silence on my end, got caught up with other projects for a bit! Glad to know this was helpful!

[aadsm]

No worries, this PR is awesome!