- Notifications
You must be signed in to change notification settings - Fork115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Refactored html-tools and htmljs to move ES6 version#385
base:release-3.0
Are you sure you want to change the base?
Conversation
Besides the pr, I found some exported function has no usage in the blaze like isNully, isTagEnsured, etc... should we stop exporting them. |
Thank you@guncebektas for this PR. Please note, that we first need to merge#382 before we can continue to work on the actual migrations. Once it's done I will review this one. |
@guncebektas I merged#382 to the |
This will be hard to achieve :) |
Hey@guncebektas sorry that was a bit fast from my side. Of course there may be design decisions from back then that will conflict with the linter. For now please only to non-breaking migrations. All other issues will be discussed during review. |
Another thing is, which I mentioned in#385 is that you should please only do one package per PR. Otherwise this gets easily out of hand and becomes impossible to review. For now please keep the scope only to the two packages you worked on. |
no problem. I will try to do my best. |
It wasn't possible to fulfill all linting rules but I tried to minimize errors. I think a review at this point will be good. In addition to this, I used my version as local packages in my project and all of my e2e tests are passing. I can trust myself and move faster after reviews.@jankapunkt |
I just focused on |
We are getting closer :-) |
Are we ok with htmljs@jankapunkt |
Once@jankapunkt approves, I will merge this. |
Is there a prefered package for the next pr? |
hey@guncebektas@StorytellerCZ this is quite a bit to review (which is why in the future there should be only one package per PR) I try to get it done asap the next days! |
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.
Generally approved, waiting for the comment on the package version bumps, otherwise this is a great step in the right direction
| Package.describe({ | ||
| name: 'html-tools', | ||
| summary: "Standards-compliant HTML tools", | ||
| version: '1.1.3', |
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.
@StorytellerCZ@denihs should PRs to 3.0 include version bumps? I'd rather keep them and at release bump them to whatever is suitable but that's not my call.
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.
ping@Grubba27
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.
Actually I think PRs probablyshouldn't include individual version bumps, it feels like this way lie merge conflicts & conflicting version number schemes for sure :D
DanielDornhardt left a comment
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.
Hi.
I've reviewed this, unfortunately it's hard to review because of the amount of stylistic changes. Which is unfortunate, because this has been recommended in the past.
We added some feedback, but basically I think we're gonna start over with the refactoring and potentially pick out the good parts from this PR as inspiration.
Thank you@guncebektas for your work! If you wanna continue to help out feel free to reach out!
| // Note that some entities don't have a final semicolon! These are used to | ||
| // make `<` (for example) with no semicolon a parse error but `&abcde` not. | ||
| var ENTITIES = { |
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.
Nit / Question:
These ENTITIES seem to have been pasted in fromhttp://www.whatwg.org/specs/web-apps/current-work/multipage/entities.json a long time ago.
Converting the quotes to another formatwould make it difficult to update this in the future by just pasting the most recent JSON fromhttps://www.whatwg.org and checking the diff on what has changed.
So I'd recommendNOT doing this for this file, at least not for the object contents.
Also: While checking this I noticed that there have been lots of updates & changes to theentities.json file of the Whatwg but I don't know what all of this does right now in the greater Blaze context?
-> Nit: Code Quality: File could use a file level comment about what it's used for, then we could reason about this more easily. But that's not for you@guncebektas just a general observation :D
| @@ -1,56 +1,55 @@ | |||
| /* eslint-env meteor */ | |||
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.
Question@jankapunkt@guncebektas : Does this have to be included in different files? Isn't there a default .eslintrc or something for blaze which should contain this?
| import { HTMLTools } from 'meteor/html-tools'; | ||
| var Scanner = HTMLTools.Scanner; | ||
| var getCharacterReference = HTMLTools.Parse.getCharacterReference; | ||
| const { Scanner } = HTMLTools; |
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.
Nit: I think the old way was more straightforward to read -const is fine but in general? Do we need this everywhere?
| Package.describe({ | ||
| name: 'html-tools', | ||
| summary: "Standards-compliant HTML tools", | ||
| version: '1.1.3', |
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.
Actually I think PRs probablyshouldn't include individual version bumps, it feels like this way lie merge conflicts & conflicting version number schemes for sure :D
packages/html-tools/parse.js Outdated
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.
🛑 OK... I think this is a blocker - re-sorting / rearranging all the functions makes this one really hard to review...why has this been rearranged@guncebektas ?
| } | ||
| _assign(TemplateTag.prototype, { | ||
| Object.assign(TemplateTag.prototype, { |
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 kind of like this one, if it does the same thing indeed :D
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 it does.
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.
| // consumes characters and emits nothing (e.g. in the case of template | ||
| // comments), we may go from not-at-EOF to at-EOF and return `null`, | ||
| // while otherwise we always find some token to return. | ||
| export function getHTMLToken (scanner, dataMode) { |
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.
Again... moving things around makes it difficult to judge the changes.
| var getDoctype = HTMLTools.Parse.getDoctype; | ||
| var getHTMLToken = HTMLTools.Parse.getHTMLToken; | ||
| const { Scanner } = HTMLTools; | ||
| const { getComment } = HTMLTools.Parse; |
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.
see above :)
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.
Sorry, hard to parse changes
@guncebektas can you please take a look on@DanielDornhardt comments? |
As requested on#367, I started to migrate code to ES6.
All tests are passing, please review and let me know for required changes.