Skip to content
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

Refactored blaze to move ES6 version #397

Open
wants to merge 8 commits into
base: release-3.0
Choose a base branch
from

Conversation

klablink
Copy link

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

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2022

CLA assistant check
All committers have signed the CLA.

@StorytellerCZ
Copy link
Collaborator

Thanks a lot @klablink! Can you please sign the CLA, so that we can proceed on this?

@klablink
Copy link
Author

Thanks a lot @klablink! Can you please sign the CLA, so that we can proceed on this?

Hi @StorytellerCZ, I have signed the CLA now.

Copy link
Collaborator

@jankapunkt jankapunkt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some things to resolve, especially the Es5 to ES6 classes and the disabled eslint warnings. Let me know if you need help at any point.

packages/blaze/attrs.js Outdated Show resolved Hide resolved
packages/blaze/attrs.js Outdated Show resolved Hide resolved
packages/blaze/attrs.js Outdated Show resolved Hide resolved
packages/blaze/backcompat.js Outdated Show resolved Hide resolved
packages/blaze/dombackend.js Outdated Show resolved Hide resolved
packages/blaze/package.js Outdated Show resolved Hide resolved
packages/blaze/template.js Outdated Show resolved Hide resolved
packages/blaze/template.js Outdated Show resolved Hide resolved
packages/blaze/template.js Outdated Show resolved Hide resolved
packages/blaze/view.js Outdated Show resolved Hide resolved
@klablink
Copy link
Author

klablink commented Nov 1, 2022

I have completed the migration of all packages, tell me if there is anything to change.

@klablink
Copy link
Author

klablink commented Nov 2, 2022

@StorytellerCZ @jankapunkt The removal of the UI package makes the iron-router package incompatible. I cannot find it in the community-managed packages, so in my opinion it should be added. What do you think?

@jankapunkt
Copy link
Collaborator

jankapunkt commented Nov 2, 2022

The removal of the UI package makes the iron-router package incompatible

This is a sign that iron router has to be updated and that there is lots of legacy code in it and maybe even a few parts that could be vulnerable (afaik iron router can be used on the server, too, right!?)

@klablink
Copy link
Author

klablink commented Nov 2, 2022

The removal of the UI package makes the iron-router package incompatible

This is a sign that iron router has to be updated and that there is lots of legacy code in it and maybe even a few parts that could be vulnerable (afaik iron router can be used on the server, too, right!?)

Right, I think many projects started with Meteor in version 0.x still use this package. We use it more client-side, server-side we use express.

@StorytellerCZ
Copy link
Collaborator

@klablink can you please split this PR into smaller ones by packages, so that we can review this more efficiently. Thank you!

@klablink
Copy link
Author

@klablink can you please split this PR into smaller ones by packages, so that we can review this more efficiently. Thank you!

Hi @StorytellerCZ I can try but it is not easy because some changes are across packages.

@klablink
Copy link
Author

@StorytellerCZ I checked the possibility of splitting PR. Unfortunately, the packages are strongly interconnected, the modification of a construct implies the revision of all references. For example, the transformation of Blaze objects into ES6 classes involves the use of the new clause in the linked sources. Now, how can we proceed to simplify the verification task?

@jankapunkt
Copy link
Collaborator

Alternatively we could split reviews between @StorytellerCZ , somebody else and me?

@StorytellerCZ StorytellerCZ linked an issue Jan 9, 2023 that may be closed by this pull request
14 tasks
@StorytellerCZ
Copy link
Collaborator

I think we need to split lint fixes from this PR. There area lot of files where the only change is that it has been linted. That should go into a separate PR, we merge that and that should reduce the number of files that need to be reviewed.

@jankapunkt
Copy link
Collaborator

Yes @StorytellerCZ good point. The biggest changes, apart from linting, are:

  • classes
  • arrow functions
  • template literals

the rest is mostly linting.

@klablink klablink removed their assignment Jan 27, 2023
@StorytellerCZ
Copy link
Collaborator

StorytellerCZ commented Sep 20, 2023

Lets split this up, and get this in asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move codebase to ES6
4 participants