43081j / id3

A JavaScript ID3 tags parser for Node & browsers.
MIT License
335 stars 63 forks source link

Rollup build #38

Closed MichaelMarner closed 5 years ago

MichaelMarner commented 5 years ago

This PR does the following:

This started as just an attempt to close #33. However, when looking through the commit history it became clear that the source files in lib were not up to date with the bundled version in dist. Several commits have edited dist/id3.js directly. So I have copied the updated code into lib to continue.

I have added lib/index.js, which sets the library up for use as before.

I have added a basic webpack build setup. Running npm run build will bundle the lib folder into dist/id3.js. I have webpack configured to export a node compatible UMD bundle. This works in Node, but could do with further testing for support in browser.

43081j commented 5 years ago

This project is largely unmaintained and has been in need of a cleanup for some time now so thanks for taking interest in it.

My main feedback here is that i'd much rather use rollup as it is much lighter than webpack and more focused on being a bundler than being a "bit of everything".

It also means we could move the sources to using ES modules (import/export rather than module.exports) but we would probably have to introduce babel to transpile down to commonjs in that case. If you're interested in having a go at that, im happy to explain what needs doing as it isn't a huge piece of work either (a few find and replaces really).

Whether we do the ESM conversion or not, i think rollup can still be used and should keep the build very light.

Would you mind also introducing an .editorconfig if you could? To ensure we're consistent with indentation and what not from now on (unfortunately we're using tabs because i didn't know any better back then..).

In reality, re-visiting this repository raises plenty of "we should do this" thoughts. If I find the time or you or someone else would like to, it would be great to clean the code base up in a separate PR (remove the filthy dataview patching, s/\t/ /g, let/const, etc.).

MichaelMarner commented 5 years ago

Where I have ended up going with this on my fork is migrating to Typescript, as I want to use it with Angular:

https://github.com/MichaelMarner/id3/tree/ts

(branch is most likely broken right now)

Not sure how you feel about Typescript, but I'll tidy up this PR and swap Webpack for Rollup anyway. Hopefully in the next day or so.

MichaelMarner commented 5 years ago

Do you have any particular rules you would like in an .editorconfig apart from using tabs for indentation?

43081j commented 5 years ago

I'm a 2-space/LF person myself but to avoid rewriting the entire code base we should probably (for now) stick to tab indentation and LF line endings:

end_of_line = lf
indent_style = tab

I use typescript in most projects i contribute to, so i'd definitely be open to moving this project to it. That would remove the need for babel as the typescript compiler will output commonjs modules for us.

I imagine there's plenty of bugs we'll uncover by strongly typing the codebase too (or places where we essentially use any and shouldn't).

If you want to submit a PR for moving to ts i would be happy to review it as i do have some preferences/ideas around that.

For this PR i suggest we try out rollup with the existing sources but limit the scope of change here to just bundle/library generation.

MichaelMarner commented 5 years ago

Have switched to Rollup to build. I have not replaced the current dist/id3.js with the rollup built one.

This probably needs further testing, but has been working find with how I've been using it (node, opening local files).

43081j commented 5 years ago

Looks good so far.

Now that we have it using rollup, i see you moved to import/export too (es modules). Was hoping we could, though this does mean we need to stick with main being dist/id3.js for now (until we move to TS).

Testing is something we're missing completely. From what I remember, I tested it manually in early stages but i may have another look into introducing a test suite this week. It is difficult because we'd need to essentially check in a valid "test" mp3 i guess.

Once we're certain everything behaves as it did before, this should probably be merged into a new 2.x branch so we can also throw typescript in there at a later point. I'll do some manual testing if i get time this week.

MichaelMarner commented 5 years ago

Hey sorry I let this one get away - what would you like me to do/add/change here?