-
Notifications
You must be signed in to change notification settings - Fork 806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ES5 to ES2015 #220
ES5 to ES2015 #220
Conversation
@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. |
book/snippets/14-MVC.es2015.js
Outdated
initialize: function() { | ||
this.set( { "src": this.defaults.src} ); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a passing comment on code-style. For many of the ES2015 versions, we should opt for the shorter arrow function syntax where possible and consider var -> const/let where it makes sense. As a sanity check, you can try running the snippet through http://lebab.io/try-it to evaluate whether a more terse form of snippet could be possible.
const Photo = Backbone.Model.extend({
// Default attributes for the photo
defaults: {
src: "placeholder.jpg",
caption: "A default image",
viewed: false
},
// Ensure that each photo created has an `src`.
initialize() {
this.set( { "src": this.defaults.src} );
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a reasonable comment. But this is a case that I told you in chat about Backbone and their documentation. But if you think so I could change that code to es2015 too.
@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
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.
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 |
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 |
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 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? |
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 |
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. |
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 :) |
Changes here LGTM. Thanks for adding in the lock file! |
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 :)
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?
It's definetely great idea. Same thing about
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?
Fixed this, added new syntax to AMD too. |
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.
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
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.
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. |
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.