Discussion: Moving `gmusic.js` to an extensible ES6 style

[MarshallOfSound]

Moved this issue to the correct repo

Current gmusic.js is a bit messy. All of our namespaces and methods are in one file, all the selectors are in that file as well. Realisticly we should have separate files for each namespace.

I would like to propose that we (me) port gmusic.js to the same style of files as gmusic-ui.js (with a couple of technicaly structure tweaks).

The main goal of this port is to

• Make the learning curve in contributing to the repo much easier
• Provide a simple API (the GMusicNamespace class) to extend the GMusic prototype
• Clean up the code in general by splitting the namespaces into modules
• Move the binding logic away from us and onto the ES6 transpilers

I'm keen to do this, just looking for feedback before I start to tackle it.

For those unfamiliar with how GMusic-UI.js works we have a base GMusicNamespace class which all our namespaces extend. You then call addMethod within the namespace to make a method public and everything else is handled behind the scenes.

/cc @gmusic-utils/gmusic-js @gmusic-utils/gmusic-ui-js

[MarshallOfSound]

Further discussion was held here (kinda)

https://github.com/gmusic-utils/meta/issues/5

[MarshallOfSound]

My rather large follow up

I'll address the points raised one by one

Current code base is relatively stable

Yes, the current code base is stable, this includes the tests. Moving to a new framework should retain our conistancy and stability as our test suite should prevent regressions.

Wider recognition by wider audience (not everyone at every company has been switching to ES6)

This I can somewhat agree with but I will address the uptake of ES6 further down

Compiled result is almost verbatim as input result (leading to simple debugging)

Although people seem to think debugging compiled ES6 is outstandingly difficult it is suprisingly easy. Lots of the variables just end up with underscores either side of their name and not much else changes.

We already have a class-esque namespace system via init functions

Why use a "class-esque" system which we wrote ourselves when we can use the proper Javascript class system with proper classes and constructors which are universally understood.

In my opinion, this is a request for stylistic comfort which is never a good reason to make a technical decision. For example, would you rewrite an entire code base from snake_case to camelCase because you used camelCase at your last company?

You misunderstand the proposal, this isn't a stylistic comfort issue. I've been comfortable with the ES5 implementation the whole time I've been in this repo. However, ES6 is the new standard of Javascript and as such shouldn't be looked at as a stylistic change rather a future proofing and getting up to date change. ES6 is the future of Javascript and it's better to get on board now than later into the game.

I should note that there is a counterpoint about organizational consistency with the same language (e.g. all repos should be in the same style). However, I have expressed my opposition to using ES6 in the past so that fragmentation has already occurred.

The reason the new repos I started are written in ES6 is the same reasoning as above. ES6 is the future of JS, starting a new repo in ES5 makes very little sense at the moment. Whereas porting an ES5 repo over to ES6 makes a lot of sense.

Breaking down into separate files

This was more of a side point and not related entirely to the move to ES6. But to clarify the intention, each namespace would be in it's own file and Selectors and methods would be contained inside a namespace.

With respect to automated documentation (e.g. ESDoc), there are plenty of non-ES6 based tools like JSDoc. The decision to write manual documentation is been a personal one; I find more flexibility from external documentation (e.g. I can provide examples) and not bloat up the code (e.g. examples can get bulky)

Yeah this was another side point, JSDoc and ESDoc actually follow the same syntax. I'm OK with either but I was just querying what we thought of that idea :+1:

Thanks for the feedback people

[twolfson]

My initial response:

• https://github.com/gmusic-utils/meta/issues/5#issuecomment-233371652
• https://github.com/gmusic-utils/meta/issues/5#issuecomment-233372262

To reiterate my sentiments, I agree that moving to ES6 is a good thing to do in the future but not now. Here's my rationale:

• ES6 support is still being explored (e.g. modules are still being standardized and we will see how well those work/how well they get adopted)
• Support is largely limited in browsers and Node.js<=4
  • http://kangax.github.io/compat-table/es6/
  • http://node.green/
• No technical benefits, currently only stylistic

I would like to defer this discussion for another year or so. At that point, implementing ES6 should actually simplify the repo (e.g. we won't need a transpiler at all, fingers crossed) but for now I am closing this discussion.

I should note that I am open to continuing other side discussions, such as breaking down the main file as directly asked by https://github.com/gmusic-utils/meta/issues/5#issuecomment-233827895 but that should be in another thread.

[MarshallOfSound]

@twolfson Although your points contain good reasoning, I feel as though they are invalid for multiple reasons.

ES6 support is still being explored (e.g. modules are still being standardized and we will see how well those work/how well they get adopted)

The core ES6 modules have been standardized and it is widely considered safe to use anything in the ES2015 pack. A large group of major open source projects already reccomend ES6 syntax in their docs and use it internally. A small list of examples include

• Electron The main tool that this library gets used in, if not the only one
• React
• Ember.js
• Angular JS 2.0
• And literally hundreds more. Just check the dependents section of the NPM page

Without wanting to play the it's not me it's you game I feel now that you're playing for a

stylistic comfort which is never a good reason to make a technical decision

Particularly when your second point is just countered with babel.. Use the tool that everyone else uses, deactivate babel when support is wide spread enough to do so. If used in electron, you don't even need babel as ES6 is natively supported. Note: Not claiming this is only used in electron, merely a point

No technical benefits, currently only stylistic

Technically this is correct, but we do get rid of browser specific quirks when using babel and the stylistic changes aren't exactly a move in the wrong direction. ES6 is the future after all.

Basically my point is, saying "We'll do it later when support is larger" when hundreds of major projects and even more minor projects are making the switch to ES6 already or already have been is kind of void.

I'll definitely open tickets for the breaking down and internal documentation but I really want this to be considered. As an organization and as the users of this library.

Specifically I want the feedback of @jostrander and @jacobwgillespie to be taken into account, and if possible for @chrismou to weight in on this. We came together in this organization for better maintainability of this library so we all could use it and I hope we can see that making this move helps us achieve those goals even more.

[chrismou]

Personally speaking, my entry into JS apps was EmberJS, so I don't have any particular hang ups about using ES6. If I'm honest it doesn't bother me much either way, but Samuel's point about current usage in popular frameworks is a fairly convincing argument that it's probably not "too early" to update the code. It's seems like it's just a decision of whether we want to spend the time doing it.

As this library specifically targets Play Music, and Google are clear that they don't "officially" support any Chrome version older than the current release and the previous version of IE/Edge/Firefox/etc (https://support.google.com/a/answer/33864?hl=en), shouldn't the only consideration around compatibility be whether those browsers support ES6, regardless of the global usage of other browsers? Ie, there's no point holding off because IE 11 doesn't support certain features, when Google themselves dropped support for it over a year ago?

[chrismou]

Oh wait, didn't someone move this chat to the meta repo? Sorry, I just clicked through and didn't check what repo I was on.

[twolfson]

@MarshallOfSound I believe you misinterpreted my points (albeit unintentionally). I will clarify:

• With respect to module support, yes the syntax is supported by tools like Babel but in a browser like Chrome I cannot run import GMusic from 'gmusic.js';. That is still being standardized and I am still hesitant about its reception (due to it still requiring dependencies to be requested in series which causes significant lag)

We can honestly go back and forth all day with ES6 being stylistic with respect to this repository. I think we need to agree to disagree on that point.

I forgot to reply to the point about tests catching regressions during the transition. Unfortunately, the transition will be done by humans and in my (and almost every other programmer's experience), this inevitably introduces human errors (i.e. bugs). Hence the typical "if it isn't broken, don't fix it" mentality.

@chrismou You have a very valid point. It lines up with my desire to wait until browser support is completed for ES6. As linked previously, here's the current support chart for ES6 in modern browsers:

http://kangax.github.io/compat-table/es6/

As mentioned at the beginning of this comment, I would also like to wait for browsers to natively support importing modules (i.e. import GMusic from 'gmusic.js';).

[MarshallOfSound]

@twolfson

With respect to module support, yes the syntax is supported by tools like Babel but in a browser like Chrome I cannot run .. That is still being standardized and I am still hesitant about its reception (due to it still requiring dependencies to be requested in series which causes significant lag)

None of that matters because Babel supports it and transpiles to an ES5 compatible syntax. And our main method of serving up GMusic currently is a browserified file that doesn't use require at all I don't see how transpilation from ES6 is a risk at all. We don't need to Chrome to support it, we simply need babel to support it. When chrome (V8) supports it we can simply drop the transpiler.

Picking apart the import feature of ES6 is irrelevant to the current discussion as we would never use import in a shipped version. We ship browserified code. And even if people require it like a normal node module import would respond identically to how require works in node at the moment (sequential importing)...

We can honestly go back and forth all day with ES6 being stylistic with respect to this repository. I think we need to agree to disagree on that point.

OK 😆

I forgot to reply to the point about tests catching regressions during the transition. Unfortunately, the transition will be done by humans and in my (and almost every other programmer's experience), this inevitably introduces human errors (i.e. bugs). Hence the typical "if it isn't broken, don't fix it" mentality.

If our tests don't catch that kind of error then maybe our tests need fixing up first. As far as I am aware our tests ensure this library operates correctly and simply dropping a new version into any of it's implementations (Radiant, GPMDP or gme) would quickly reveal if there is anything the tests didn't catch. If we don't have faith in our own tests then we have a much larger problem than just "should be move to ES6".

@chrismou

shouldn't the only consideration around compatibility be whether those browsers support ES6, regardless of the global usage of other browsers? Ie, there's no point holding off because IE 11 doesn't support certain features, when Google themselves dropped support for it over a year ago?

I stand by my "everyone is comfortable using babel, people (including me) have done risk analysis's for using babel, if they decided it was OK what makes us so different". Babel solves all "this browser doesn't support XXXX" problems as babel standardizes ES6 support across browsers. When ES6 support is wide enough spread in non-chrome browsers (Chrome basically has it all anyway) we can simply drop the transpiler.

To summarize: I fail to see any negatives from making this move apart from the effort required to do so (which I have already said I am willing to put in) and the positives now and potential future positives (when ES6 becomes fully supported in browsers and we can drop the transpiler) are quite significant.

[twolfson]

While you might be comfortable making the changes, however it isn't exclusively you that needs to maintain them.

We created this organization and introduced collaborators to make patching bugs a faster process. This seems contradictory to those efforts as there is only potential to introduce bugs and no potential for anything to be resolved that cannot be resolved independently.

I know that we can use Babel but we can also use Browserify (which we do). Moving between tools doesn't make any sense when nothing is wrong. The only rational time for me to agree with moving to ES6 is when it would simplify the repo (and maintenance process), not complicate it.

[twolfson]

Maybe I should ask the following question to better understand your perspective. What do you want to use that you cannot get from the current setup?

[MarshallOfSound]

I know that we can use Babel but we can also use Browserify (which we do). Moving between tools doesn't make any sense when nothing is wrong. The only rational time for me to agree with moving to ES6 is when it would simplify the repo (and maintenance process), not complicate it.

We wouldn't be moving between tools, we would still use browserify to pull all our files into one web compatable distributable JS file.

Maybe I should ask the following question to better understand your perspective. What do you want to use that you cannot get from the current setup?

The main point was at the top of this whole thread, extensability. Hacking into a prototype object is just wrong to me on so many levels for extending a tool. The wrapper that performs this task in gmusic-ui is a much nicer interface for extending GMusic and I want to see that upstream.

I also want to see the adoption of new technologies as appropriate in repos we maintain.

We created this organization and introduced collaborators to make patching bugs a faster process.

The modularity thing (I know I need to raise another ticket) and ES6 as a whole is easier to read (subjective) and what a lot of people who come to this repo for bugfixes use anyway. GPMDP and Radiant 2.0 are both entirely ES6 and that accounts for a significant quanitity of the bug reports and fixes.

Personally I see no extra effort / difference in maintaining an ES6 repo as opposed to an ES5 repo and could even argue that ES6 is easier to maintain due to how we currently bind things all over the place and ES6 just does all that for us and is universally understood as doing so.

[twolfson]

So to reiterate, you want to shift the architecture from objects/namespaces to constructors/classes?

[MarshallOfSound]

@twolfson Pretty much. That's basically all the ES6 we can use in this repo.

There are some other neat features we can use like Template Literals and proper this scoping just comes with the package. The actual logic and high level structure wouldn't change that much. (if at all)

[twolfson]

Moving to constructors can be done without ES6. I previously explored changing to this to be per-namespace (and a this.parent to access the higher level or something like that).

In the current lib/main.js, we have 77 instances of this being called (excluding that). Making such a change will be tedious to review and won't fix any cognitive overhead (developers will still be in a tree structure and need to think about calling this.parent instead of this[namespace] but it's the same problem).

Maybe the finer problem is that we naively called the namespaces prototypes when they aren't that, would renaming proto/protoKey to namespace/namespaceKey resolve the issue?

[MarshallOfSound]

@twolfson Not really. And moving to ES6 will solve that cognitive overhead as (at least with the 4 namespaces I've kind of ported already) all this calls are ONLY to the current namespace. Calling across namespaces is a strict no no. Selectors are passed through a constants file and everything just clicks together.

Maybe I should push the namespaces I did before raising this so you can see how it looks?

[twolfson]

Not really what? There are 77 separate instances of this in unrelated code chunks. This essentially means you are touching the entire file (i.e. 500 lines of code) and at least 1 of us is reviewing that.

There is inevitably going to be something that goes wrong and I don't want to be the one that has to clean up the mess. Especially when it's avoidable in the first place.

From a perspective of trust/respect, I don't believe you can guarantee you will be there to take responsibility. There have been plenty of PRs where I reply promptly and you become inactive for weeks or months.

On Jul 22, 2016 02:02, "Samuel Attard" notifications@github.com wrote:

@twolfson https://github.com/twolfson Not really. And moving to ES6 will solve that cognitive overhead as (at least with the 4 namespaces I've kind of ported already) all this calls are ONLY to the current namespace. Calling across namespaces is a strict no no. Selectors are passed through a constants file and everything just clicks together.

Maybe I should push the namespaces I did before raising this so you can see how it looks?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/gmusic-utils/gmusic.js/issues/47#issuecomment-234419805, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3FWE0keUmRwNOT4GDLESvcw6s6QwIOks5qYAiogaJpZM4JRcc0 .

[MarshallOfSound]

Not really what?

That was in reference to the question "would renaming proto/protoKey to namespace/namespaceKey resolve the issue?"

There have been plenty of PRs where I reply promptly and you become inactive for weeks or months.

I have an internal ranking system in my inbox. Feedback on PR's for features don't take as high a level of priority as a critical bug in another project. If critical bugs appear in gmusic which I doubt they would I, would drop everything to fix them. Not in the least because I have tens of thousands of users who would be critically affected by such a bug.

If this org isn't willing to take this supposed risk (which in my eyes is minimal considering the code coverage of both our tests and usage) then I will be forking downstream into a GPMDP org repository and making this move.

Yes reviewing such a massive change will be hard, and quite possibly take a very long time before everyone is 100% satisfied that things aren't going to fall apart. But as mentioned before this move WILL happen at some point to keep up with where JS is moving as a language. To use an earlier example as my PR to completely overhaul the test suite, although that PR was a nightmare to review and a nightmare to write. The org as a whole is better off for it.

I'm still working on the move offline and I hope this discussion can continue while I make progress. Where my progress will end up will be decided at some point.

[twolfson]

I think a fork is your best bet as I currently don't have the time to review such a sizable PR and I wouldn't be comfortable on it being landed without my eyes on it since it touches everything.

On Jul 22, 2016 02:35, "Samuel Attard" notifications@github.com wrote:

Not really what?

That was in reference to the question "would renaming proto/protoKey to namespace/namespaceKey resolve the issue?"

There have been plenty of PRs where I reply promptly and you become inactive for weeks or months.

I have an internal ranking system in my inbox. Feedback on PR's for features don't take as high a level of priority as a critical bug in another project. If critical bugs appear in gmusic which I doubt they would I would drop everything to fix them. Not in the least because I have tens of thousands of users who would be critically affected by such a bug.

If this org isn't willing to take this supposed risk (which in my eyes is minimal considering the code coverage of both our tests and usage) then I will be forking downstream into a GPMDP org repository and making this move.

Yes reviewing such a massive change will be hard, and quite possibly take a very long time before everyone is 100% satisfied that things aren't going to fall apart. But as mentioned before this move WILL happen at some point to keep up with where JS is moving as a language. To use an earlier example as my PR to completely overhaul the test suite, although that PR was a nightmare to review and a nightmare to write. The org as a whole is better off for it.

I'm still working on the move offline and I hope this discussion can continue while I make progress. Where my progress will end up will be decided at some point.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gmusic-utils/gmusic.js/issues/47#issuecomment-234424354, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3FWEHJnJ3UXEJewy8bq9sW4gbBGNSpks5qYBBNgaJpZM4JRcc0 .

[jacobwgillespie]

So, I thought it would be nice to have some code to look at while we're discussing this, and a port to ES6 isn't really difficult, so here you go: #48

This is (I believe) the minimal port to ES6, and should be very straightforward to review. A few notes:

• we've been mixing a discussion of library architecture with language version. IMO I'd love to see an ES6 class-based architecture, but that's separate.
• it is easy to integrate babel with the existing browserify build
• we're getting some sanity checks for free with let and const, as well as prevention of accidental errors surrounding ambiguous this with arrow functions
• we're now automatically getting better stacktraces as we're getting automatic named functions in the dist
• developer happiness is a nice to have, but it is nice to have ;)

[twolfson]

I feel like my arguments are being ignored and now I am starting to feel like the time/effort I have put into this repo is being threatened. I am going to do something selfish and remove collaboration rights for everyone else for the weekend.

I think we are getting fixated on what people like about ES6 and not listening to the other side of the discussion.

Maybe take this time to consider things from my perspective:

• It took 3 days to rip/clean up/formalize 1.0.0
• This included adding a very boring test suite to guarantee we can refactor without issue
• Took even more days to remove majority of inconsistencies and bad habits from repo
• Handled Google's Polymer transition with 1 day turnaround
• Opened repo to others while being skeptical as I had never worked with them before
• Now a repo which I have worked hard to maintain is being critiqued for decisions that I thought for a while about and those reasons are being ignored

Maybe this relationship isn't working out, maybe I should move gmusic.js back to a personal repo (I can reclaim google-music and you keep gmusic.js) and you can keep the organization.

[jacobwgillespie]

I apologize if I've made you feel like your contributions aren't valued; they most definitely are. gmusic.js has made my burden of updating Radiant significantly easier as I can just update the dependency when Google changes their UI.

It sounds like moving gmusic.js (google-music) to a separate repo might be a good path forward. You obviously have strong design decisions you'd like to adhere to (and nothing against that, I can relate, and it usually leads to strong software as a result). Open source exists for this kind of thing, too; we can collaborate if aligned and build different architectures as we see fit.

I think @MarshallOfSound, @jostrander, and myself would all like to move this project to ES6 (with varying different architecture opinions). I'll leave the "breakup" decision to you, but if you feel the need to suspend the contribution rights of the other repository members, I'd think it might be a good idea. :)

[twolfson]

When I said take the weekend to think about it, I meant take the weekend to think about it =P I'm going to revisit this thread on Monday at the earliest. Have a good weekend y'all ✌

[twolfson]

Alright, I took the weekend to think about it and I am in agreement with the fork. I will open another issue to discuss the process.

[twolfson]

Closing this issue as discussed; it will be completed in a fork on gmusic-utils.