Closed guar47 closed 6 years ago
@guar47 - code is sharp so far. Let me know if you want me look at anything.
@jserrao Hi! Yes, I'm still on the way. I finished 0 - 5 chapters you could see the files. I have some doubts about reimplementation module and observer. Does it look right?
Let me reread those sections in the book and compare them with your code.
@addyosmani Hey Addy and guys!
Here my report on work that I did. In most cases, I went along the way to change the code, so as not to change the text of the book. I also tried to create a consistent modern style, mostly with prettier and eslint help. In the es5
files I changed nothing, it's looks just like in book now.
All of this is just my vision of es2015 version and I want to hear your comments and fix what I missed.
Global changes:
Now about the book's chapters. I added links to code commits to make it easier for you to comment if you'll need to. Because the whole PR is pretty big for convenient navigation.
export/import
keywords, except Dojo
, ExtJS
, YUI
. There I just changed the syntax. I also, changed one for loop on reduce()
method in 2 chapter:
2 chapter.es5
2 chapter.es2015+
3 chapter.es5
3 chapter.es2015+classes
and export/import
, but maybe the result is not the same. For example, we lost privacy of random, because it is in constructor()
now:
4 chapter.es5
4 chapter.es2015+extends
and super()
here instead of the extend()
method. Also, I used the classes and did my best for consistent reimplementation:
5 chapter.es5
5 chapter.es2015+extends
and classes
instead of prototype assignment. And in Facade I used import/export
again. I also used the static
method in some cases where I thought it's appropriate (6 chapter - Mediator and 10 chapter - Factory).
6 chapter.es5
6 chapter.es2015+
7 chapter.es5
7 chapter.es2015+
8 chapter.es5
8 chapter.es2015+
9 chapter.es5
9 chapter.es2015+
10 chapter.es5
10 chapter.es2015+classes
and extends
to remove the .call()
method and escape from Object.create()
on prototype. I also used Object.assign()
instead of underscore's extend()
method. Also, in the augment
function I used map()
, Object.getOwnPropertyNames()
to escape the iteration over property chain. And rest
operator to remove arguments
. I also found this reimplementation http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/, but this is more than code changes, it'll change the text of the book, so I decided not to stick with this one.
11 chapter.es5
11 chapter.es2015+extends
inheritance and removed the custom extend
function.
12 chapter.es5
12 chapter.es2015+Backbone.js
, KnockoutJS
, and Spine.js
as well. I checked all their current documentation and it's all with old es5 code syntax. Also, I think they not so popular nowadays, so I decided to remain code examples as they were. Perfect solution will be to placed examples with a new implementation of http://todomvc.com/ (which used here for most examples) for example es2015 TodoMVC, but again it'll entail text changes, so now I just left old code. Of course, I could change the syntax here too (like const, arrow functions etc.), but maybe it's not necessary, but if it needs to I'll do it.
14 chapter.es5
14 chapter.es2015+
15 chapter.no-changes
16 chapter.no-changes
17 chapter.no-changesES.next
and now we had different implementation, am I right? For example, we don't have a module
, public
, private
keywords nowadays (only in TypeSript but I don't think it's relevant) and looks like import/export
is different here, so I decide do not touch this section too.
However, 19 section about CommonJS still relevant in Node.js so I changed the syntax in this one to a modern version. I also didn't know about UMD, but I changed that too, except the examples from the core code.
18 chapter.no-changes
19 chapter.es5
19 chapter.es2015+
20 chapter.es5
20 chapter.es2015+
21 chapter.no-changes1. Single global variables
first snippet maybe not correct, I changed it a bit. And in Automating nested namespacing
in the first snippet I changed the key name from 2d
on to2d
.
31 chapter.es5
31 chapter.es2015+
32 chapter.es5
32 chapter.es2015+That's all that I could say. If you have some questions or comments what I should do in a different way, please let me know.
PS. I also added package-lock.json
if you don't mind.
I tried to change the implementation of Singleton with classes and export/import, but maybe the result is not the same. For example, we lost privacy of random, because it is in constructor() now:
Would it make sense to try re-introducing a sense of privacy for random using WeakMaps?
https://www.sitepen.com/blog/2015/03/19/legitimate-memory-efficient-privacy-with-es6-weakmaps/
Raushma outlined a few alternative patterns for privacy in classes over here too that I found an interesting read http://2ality.com/2016/01/private-data-classes.html
Observer and PubSub was the tough ones for me, I tried to use extends and super() here instead of the extend() method. Also, I used the classes and did my best for consistent reimplementation:
That makes sense. I don't think we're a million miles off here.
I've been surveying some of the other implementations folks have experimented with and we might want to gut check if there are any useful tweaks they might inform us of:
https://pawelgrzybek.com/the-observer-pattern-in-javascript-explained/
I also found this reimplementation http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/, but this is more than code changes, it'll change the text of the book, so I decided not to stick with this one.
I was going to suggest Justin's patterns but then saw you already evaluated them! This might be the cleanest approach for mixins today but you're correct that it would change the text substantially. Would you mind if we created a variation using Justin's mixing approach (e.g es2015.version2.js) and I can take an action item to rewrite the book text to factor it in?
I changed the implementation of Decorator with extends inheritance and removed the custom extend function.
These changes look reasonable. A bit of an opinion question for you: given JS now has an official decorators proposal, do you think it makes sense to reference it in the book?
I wrote an article with code samples embedded in there before on the proposal and I don't think the syntax in https://github.com/tc39/proposal-decorators has massively changed.
Here's my post https://medium.com/@2508e4c7a8ec/76ecb65fb841
In the Modular section, we have a two chapter without changes (18 and 21) because 18 chapter only about AMD modules and RequireJS, which again, I'm not sure they relevant nowadays. Correct me if I wrong. And 21 chapter maybe I didn't catch the understanding. But it seems to me like Harmony was draft in ES.next and now we had different implementation, am I right? For example, we don't have a module, public, private keywords nowadays (only in TypeSript but I don't think it's relevant) and looks like import/export is different here, so I decide do not touch this section too.
The Harmony modules section was indeed written many years ago before the final syntax stabilized. I'll need to rewrite the text for that section pretty extensively.
What I would like for the code samples is for us to completely ignore the older Harmony modules syntax and try to implement the samples with the final ES Modules syntax (http://2ality.com/2014/09/es6-modules-final.html) in preparation for that rewrite. Would that be okay?
I know AMD is still used by a few thousand sites despite being older, so some updated syntax examples of those patterns make sense I think.
UMD sgtm. We continued evolving the UMD patterns/standards over in https://github.com/umdjs/umd over the last few years, so just updating the syntax for those samples sounds reasonable.
In Flyghtweight I didn't like the Function.prototype assignment at the start of the chapter, but nothing come to my mind how to change this. I also slightly change the implementation with classes where I could and used the functions where couldn't.
The changes you made to the pattern lgtm. I haven't seen as much interest or use of this pattern in the JavaScript community over the last few years so there are few "other interpretations" of it available for us to compare to. For now, let's run with what you put together :)
And finally we have the namespacing section, which I tried to change, but the only syntax in most cases. Also, I think in 1. Single global variables first snippet maybe not correct, I changed it a bit. And in Automating nested namespacing in the first snippet I changed the key name from 2d on to2d.
Changes here LGTM.
Thanks for adding in the lock file!
Would it make sense to try re-introducing a sense of privacy for random using WeakMaps? https://www.sitepen.com/blog/2015/03/19/legitimate-memory-efficient-privacy-with-es6-weakmaps/ Raushma outlined a few alternative patterns for privacy in classes over here too that I found an interesting read http://2ality.com/2016/01/private-data-classes.html
Yes, thank you. It is really great examples. But I realized I use module system in Singleton, so I just took out the randomNumber from class declaration. I could write the variations of different examples of data privacy in the module pattern in another file if you want to. Also, I think all examples maybe will go away when https://github.com/tc39/proposal-class-fields will be released :)
I was going to suggest Justin's patterns but then saw you already evaluated them! This might be the cleanest approach for mixins today but you're correct that it would change the text substantially. Would you mind if we created a variation using Justin's mixing approach (e.g es2015.version2.js) and I can take an action item to rewrite the book text to factor it in?
Yes, surely. I'll do that in a separated file. But what examples should I add do you think? I could try to implement your examples with Justin's pattern or I could place his examples here, what do you think?
These changes look reasonable. A bit of an opinion question for you: given JS now has an official decorators proposal, do you think it makes sense to reference it in the book?
It's definetely great idea. Same thing about proposal-class-fields
that I mentioned before. I think it must be mentioned in the book. And also if it will be a new version of the book in the future it will be simple to make changes with that links.
What I would like for the code samples is for us to completely ignore the older Harmony modules syntax and try to implement the samples with the final ES Modules syntax (http://2ality.com/2014/09/es6-modules-final.html) in preparation for that rewrite. Would that be okay?
Yes it definetely okay, but I use new ES Modules all across the book, starting in 2 chapter - the Module pattern. And in different parts of snippets where we use data privacy especially. So, if I did this here I think it'll be duplication, what do you think?
I know AMD is still used by a few thousand sites despite being older, so some updated syntax examples of those patterns make sense I think.
Fixed this, added new syntax to AMD too.
I could write the variations of different examples of data privacy in the module pattern in another file if you want to.
Only if it's not too much trouble! I know that this project has already gone on for a while, but if there's time, I'd love if we could include a few variations.
Also, I think all examples maybe will go away when https://github.com/tc39/proposal-class-fields will be released :)
This sounds pretty fair. Yeah, I think we're not too far away from having class fields. The implementation in V8 might be behind flags at the moment: https://twitter.com/_gsathya/status/950494157394423808
I could try to implement your examples with Justin's pattern or I could place his examples here, what do you think?
If we could try to implement the existing examples with Justin's pattern and maybe link to his article that could work? I'm happy to chat with him about including his (which we can do at a later date) but for now maybe just using our own examples makes senes.
So, if I did this here I think it'll be duplication, what do you think?
Reviewing, I completely agree. This would be a duplicate. Let's avoid dupes!
@addyosmani Hey Addy! I added two files, with new Mixin implementation and Module.
The Mixin is a bit different because we use native language here, so we can't mix the certain methods here like in augment
method earlier, so I just implement with all of them.
Let me know what do you think about it.
Thanks once again for your hard work on this!
This PR is for #218 issue. Now, it's work in progress. I'll describe a structure and comments syntax that I used for more readable files and more comfortable insert them in the book later.
Path:
/book/snippets/files
Filenames:[chapter's number]-[chapter-name].es5.js
- there are old snippets as is.[chapter's number]-[chapter-name].es2015.js
- there are new snippets with comments.[chapter's number]-[chapter-name].es5-no-changes.js
- there are no changes from es5 for some reasons.Comments:
//********************** Snippet 1 **********************//
- snippet's number in each subchapter// [ES2015+]
- Comment related to new syntax. Often they are duplicatedAlso, I added a few usual comments with my thoughts about snippets or related text in the book.