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

Move codebase to ES6 #367

Open
4 of 14 tasks
jankapunkt opened this issue Apr 14, 2022 · 15 comments · May be fixed by #397
Open
4 of 14 tasks

Move codebase to ES6 #367

jankapunkt opened this issue Apr 14, 2022 · 15 comments · May be fixed by #397
Labels
good-first-issue Good first issue or something that should is nice to do.
Milestone

Comments

@jankapunkt
Copy link
Collaborator

jankapunkt commented Apr 14, 2022

This would be a big task but I think there will be lots of benefits in terms of code-readability (especially with template-literals) and a few performance improvements either.

This could be also distributed to PR-per-package:

In an optimum there would be no changes to the functionality at all.

@StorytellerCZ StorytellerCZ added this to the Blaze 3.0 milestone Apr 14, 2022
@StorytellerCZ StorytellerCZ added the good-first-issue Good first issue or something that should is nice to do. label Apr 15, 2022
@StorytellerCZ
Copy link
Collaborator

We could also go one step further and move to TypeScript. 😄

@jankapunkt
Copy link
Collaborator Author

We could but I have no TS experience at all so to me it would take much more time to get into the ts thinking

@StorytellerCZ
Copy link
Collaborator

Alright let's take it step by step.

@jankapunkt
Copy link
Collaborator Author

I think a good starting point for moving towards TS would be to add type definitions at least, so intellisense can pick it up better.

@rashmod
Copy link

rashmod commented Jul 18, 2022

I'm willing to do this.
I'm new to open source and this seems like a good issue to resolve. Is there anything that I must know about this project or anything else?

@jankapunkt
Copy link
Collaborator Author

Great @rashmod !!!

Please do one PR per package (Thie repo contains multiple packages). Since you are new it may be a good start by using a smaller package.

Another thing that's still not discussed is which linter we use cc @StorytellerCZ @denihs

@rashmod
Copy link

rashmod commented Jul 19, 2022

Okay, then I'll start with the smaller packages. But when you say convert to es6 are you just talking about changing the var to let const or anything more. I want to be more explicit about the scope of this thing before starting.

@jankapunkt
Copy link
Collaborator Author

I woul say use const and let and all newer Methods from Object / Array and String that became available since ES6 (where applicable).

Also favour destructuring and named parameters incl. parameter defaults where applicable.

Note: arrow functions should be used with care since there are many binding going in Blaze with functions using this a lot.

Don't worry, just start and we will review anyway. If there are things left to be improved etc. we will let you know and you will have enough support to get this done right 👍

@rashmod
Copy link

rashmod commented Jul 20, 2022

I will start working on this now. But @jankapunkt you were talking about the linter to use.....?

@jankapunkt
Copy link
Collaborator Author

I just checked and the main Meteor project uses the @quave/quave config (see their package.json) which is maintained by @filipenevola so I think we should also adapt to use it.

What do you think @StorytellerCZ @denihs

@denihs
Copy link
Contributor

denihs commented Jul 22, 2022

No objection from me

This was referenced Jul 22, 2022
@rashmod
Copy link

rashmod commented Jul 24, 2022

I'm having some issues while setting up the local env. I used nvm to change to node v14.20.0 and somehow installed meteor (faced many problems with this too). But when following the steps in the readme meteor npm install for setting local env I get the error saying package.json not found.

sidenote: my primary cli git bash does not work with meteor. "bash: meteor: command not found" even tho it works using PowerShell or cmd

@jankapunkt
Copy link
Collaborator Author

I am unfortunately only on Linux and MacOS so I cannot be of much help regarding windows setup. Have you checked the Meteor forums about these issues? IIRC there should be a few solved topics on windows issues regarding installation of Meteor.

Can anyone else help out here?

@rashmod
Copy link

rashmod commented Jul 30, 2022

The error I keep getting is that there is no package.json file. And according to my beginner-level understanding, you need a package.json to do npm install and stuff

@jankapunkt
Copy link
Collaborator Author

You need to move into the test-app directory. It contains the package.json file. The contribution.md file covers the commands you can use there. This is to separate the test project from the packages.

@jankapunkt jankapunkt pinned this issue Aug 17, 2022
@klablink klablink linked a pull request Oct 13, 2022 that will close this issue
@StorytellerCZ StorytellerCZ linked a pull request Jan 9, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good first issue or something that should is nice to do.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants