Refactored html-tools and htmljs to move ES6 version #385

guncebektas · 2022-07-25T14:37:02Z

As requested on #367, I started to migrate code to ES6.
All tests are passing, please review and let me know for required changes.

guncebektas · 2022-07-25T14:39:35Z

Besides the pr, I found some exported function has no usage in the blaze like isNully, isTagEnsured, etc... should we stop exporting them.

jankapunkt · 2022-07-25T15:27:05Z

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.

jankapunkt · 2022-07-25T20:06:57Z

@guncebektas I merged #382 to the master branch so you should pull it in to use it to lint your package. Please make sure there are no remaining lint errors. You can read in the CONTRIBUTING file how to use the linter.

guncebektas · 2022-07-25T20:32:42Z

This will be hard to achieve :)

jankapunkt · 2022-07-25T21:15:07Z

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.

jankapunkt · 2022-07-25T21:17:07Z

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.

guncebektas · 2022-07-25T21:17:36Z

no problem. I will try to do my best.

guncebektas · 2022-07-25T21:54:32Z

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.
All tests of the project are passing.

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

packages/htmljs/html.js

packages/htmljs/visitors.js

jankapunkt · 2022-07-26T08:19:33Z

I just focused on htmljs package for now. My most comments focus on using ES6 builtin functions and syntax in favour of old or depracated syntaxes. Please let's focus first only on htmljs before we continue to html-tools package.

packages/html-tools/utils.js

- default added - continue usage removed

packages/html-tools/package.js

jankapunkt · 2022-08-03T07:16:09Z

We are getting closer :-)

guncebektas · 2022-08-27T15:17:50Z

Are we ok with htmljs @jankapunkt

StorytellerCZ · 2022-08-28T20:55:41Z

Once @jankapunkt approves, I will merge this.

guncebektas · 2022-08-29T07:57:37Z

Is there a prefered package for the next pr?

jankapunkt · 2022-08-30T05:51:38Z

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!

jankapunkt

Generally approved, waiting for the comment on the package version bumps, otherwise this is a great step in the right direction

jankapunkt · 2022-08-17T05:18:44Z

packages/html-tools/package.js

 Package.describe({
  name: 'html-tools',
-  summary: "Standards-compliant HTML tools",
-  version: '1.1.3',


@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.

ping @Grubba27

Actually I think PRs probably shouldn't include individual version bumps, it feels like this way lie merge conflicts & conflicting version number schemes for sure :D

DanielDornhardt

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!

DanielDornhardt · 2023-04-27T08:13:56Z

packages/html-tools/charref.js

 // Note that some entities don't have a final semicolon!  These are used to
 // make `&lt` (for example) with no semicolon a parse error but `&abcde` not.
-
-var ENTITIES = {


Nit / Question:

These ENTITIES seem to have been pasted in from http://www.whatwg.org/specs/web-apps/current-work/multipage/entities.json a long time ago.

Converting the quotes to another format would make it difficult to update this in the future by just pasting the most recent JSON from https://www.whatwg.org and checking the diff on what has changed.

So I'd recommend NOT 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 the entities.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

DanielDornhardt · 2023-04-27T08:16:24Z

packages/html-tools/charref_tests.js

@@ -1,56 +1,55 @@
+/* eslint-env meteor */


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?

DanielDornhardt · 2023-04-27T08:18:14Z

packages/html-tools/charref_tests.js

 import { HTMLTools } from 'meteor/html-tools';

-var Scanner = HTMLTools.Scanner;
-var getCharacterReference = HTMLTools.Parse.getCharacterReference;
+const { Scanner } = HTMLTools;


Nit: I think the old way was more straightforward to read - const is fine but in general? Do we need this everywhere?

DanielDornhardt · 2023-04-27T08:34:36Z

packages/html-tools/package.js

 Package.describe({
  name: 'html-tools',
-  summary: "Standards-compliant HTML tools",
-  version: '1.1.3',


Actually I think PRs probably shouldn't include individual version bumps, it feels like this way lie merge conflicts & conflicting version number schemes for sure :D

DanielDornhardt · 2023-04-27T08:38:01Z

packages/html-tools/parse.js

🛑 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 ?

DanielDornhardt · 2023-04-27T10:19:37Z

packages/html-tools/templatetag.js

 }

-_assign(TemplateTag.prototype, {
+Object.assign(TemplateTag.prototype, {


I kind of like this one, if it does the same thing indeed :D

Yes it does.

https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_assign

DanielDornhardt · 2023-04-27T10:24:16Z

packages/html-tools/tokenize.js

-// 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) {


Again... moving things around makes it difficult to judge the changes.

DanielDornhardt · 2023-04-27T10:25:04Z

packages/html-tools/tokenize_tests.js

-var getDoctype = HTMLTools.Parse.getDoctype;
-var getHTMLToken = HTMLTools.Parse.getHTMLToken;
+const { Scanner } = HTMLTools;
+const { getComment } = HTMLTools.Parse;


see above :)

packages/htmljs/html.js

DanielDornhardt · 2023-04-27T13:08:14Z

packages/htmljs/html.js

Sorry, hard to parse changes

StorytellerCZ · 2023-11-27T14:08:40Z

@guncebektas can you please take a look on @DanielDornhardt comments?

to ES6 v1

d9c3ada

jankapunkt mentioned this pull request Jul 25, 2022

Move codebase to ES6 #367

Open

14 tasks

guncebektas added 2 commits July 25, 2022 20:49

htmljs to ES6 v1 (readme.md)

48b6c77

html-tools to ES6 v1

895c109

Merge branch 'meteor:master' into master

e65274b

guncebektas and others added 2 commits July 26, 2022 00:07

automatic eslint fixes

24ed805

Merge branch 'meteor:master' into master

a8cee70

guncebektas added 2 commits July 26, 2022 00:44

manual eslint fixes

836c76e

Merge remote-tracking branch 'origin/master'

366d397

jankapunkt added this to the Blaze 3.0 milestone Jul 26, 2022

jankapunkt changed the title ~~Refactored to move ES6 version~~ Refactored html-tools and htmljs to move ES6 version Jul 26, 2022

jankapunkt requested changes Jul 26, 2022

View reviewed changes

guncebektas added 4 commits July 26, 2022 16:46

add: /* eslint-env meteor */ to the 1st line

e18db10

linting v2

b6d8d77

fix: step back on visit methods

e2bed38

add: eslint disable for rule "array destructuring"

8bde88f

guncebektas requested a review from jankapunkt July 29, 2022 11:41

zodern reviewed Jul 29, 2022

View reviewed changes

packages/html-tools/utils.js Outdated Show resolved Hide resolved

guncebektas added 3 commits August 2, 2022 17:05

eslint:

8e47093

- default added - continue usage removed

fix: readability

7ebe904

fix: scoped functions moved to outside for performance

b077fc7

Merge branch 'meteor:master' into master

cc378df

StorytellerCZ changed the base branch from master to release-3.0 August 3, 2022 06:45

StorytellerCZ requested changes Aug 3, 2022

View reviewed changes

packages/html-tools/package.js Outdated Show resolved Hide resolved

fix: version increased

3bd7704

StorytellerCZ requested a review from zodern August 11, 2022 16:17

StorytellerCZ approved these changes Aug 11, 2022

View reviewed changes

Merge branch 'release-3.0' into master

bc17f49

jankapunkt assigned guncebektas Oct 29, 2022

jankapunkt added the hacktoberfest label Oct 29, 2022

jankapunkt approved these changes Oct 29, 2022

View reviewed changes

StorytellerCZ removed the hacktoberfest label Nov 17, 2022

Merge branch 'release-3.0' into master

b388410

DanielDornhardt reviewed Apr 27, 2023

View reviewed changes

Merge branch 'release-3.0' into master

b6a0019

Merge branch 'release-3.0' into master

51c5592

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored html-tools and htmljs to move ES6 version #385

Refactored html-tools and htmljs to move ES6 version #385

guncebektas commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

guncebektas commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 26, 2022

jankapunkt commented Aug 3, 2022

guncebektas commented Aug 27, 2022

StorytellerCZ commented Aug 28, 2022

guncebektas commented Aug 29, 2022

jankapunkt commented Aug 30, 2022

jankapunkt left a comment

jankapunkt Aug 17, 2022

StorytellerCZ Nov 17, 2022

DanielDornhardt Apr 27, 2023

DanielDornhardt left a comment

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

StorytellerCZ Apr 27, 2023

StorytellerCZ Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

DanielDornhardt Apr 27, 2023

StorytellerCZ commented Nov 27, 2023

Refactored html-tools and htmljs to move ES6 version #385

Are you sure you want to change the base?

Refactored html-tools and htmljs to move ES6 version #385

Conversation

guncebektas commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

jankapunkt commented Jul 25, 2022

guncebektas commented Jul 25, 2022

guncebektas commented Jul 25, 2022

jankapunkt commented Jul 26, 2022

jankapunkt commented Aug 3, 2022

guncebektas commented Aug 27, 2022

StorytellerCZ commented Aug 28, 2022

guncebektas commented Aug 29, 2022

jankapunkt commented Aug 30, 2022

jankapunkt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielDornhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StorytellerCZ commented Nov 27, 2023