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

Use Javascript class syntax #319

Merged
merged 2 commits into from
Sep 12, 2020
Merged

Use Javascript class syntax #319

merged 2 commits into from
Sep 12, 2020

Conversation

hiddewie
Copy link
Contributor

@hiddewie hiddewie commented Sep 12, 2020

Preview the diff with whitespace disabled! direct link

This PR

Cleans up the definition of backend classes massively, by using the Javascript class syntax. This allows:

  • class A instead of .prototype
  • constructor and super instead of static calls to parent prototypes.
  • A extends B instead of util.extends(...)

@hiddewie hiddewie marked this pull request as ready for review September 12, 2020 13:54
@yohanboniface yohanboniface merged commit 350d19d into kosmtik:master Sep 12, 2020
@yohanboniface
Copy link
Member

Thanks a lot for this work!
Can you also have a look at plugins that would inherit from core classes ? From the top of my head I can remember kosmtik-tiles-export and kosmtik-mbtiles-export at least.

Otherwise I'll have a look myself, but I'm a bit in a rush there days :(

@hiddewie hiddewie deleted the class-syntax branch September 12, 2020 16:56
@hiddewie
Copy link
Contributor Author

hiddewie commented Sep 12, 2020

@yohanboniface Thanks for reviewing + merging so quickly :)

Yes, definitely. It would be nice to also have the plugins up-to-date considering the code style. I will make some PRs in those plugin repos.

After that, it would be nice to have NPM releases to let users (also see #309) without intimate Javascript/NodeJS knowledge install the package using the instructions in the readme.

It would also be nice to have Travis build PRs / branches, such that feedback is available in Github PRs about the build status, before merging the PR.

@hiddewie
Copy link
Contributor Author

@yohanboniface I went through the entire list of plugins, and updated the (backend) code to the class syntax whereever possible.

@yohanboniface
Copy link
Member

Fantastic work, thanks!

@yohanboniface
Copy link
Member

After that, it would be nice to have NPM releases to let users (also see #309) without intimate Javascript/NodeJS knowledge install the package using the instructions in the readme.

Sure! Missing time to do it right away, I'll try next week, same for Travis :s

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

Successfully merging this pull request may close these issues.

None yet

2 participants