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

Update to node 14 and upgrade libraries #208

Closed
wants to merge 1 commit into from
Closed

Update to node 14 and upgrade libraries #208

wants to merge 1 commit into from

Conversation

HarelM
Copy link
Member

@HarelM HarelM commented Jul 13, 2021

Launch Checklist

Upgrade development environment to be up-to-date.

This is a rework of PR #190.
@astridx feel free to send PR into this branch (node-14). I'll try to do my best to redo the work...

  • confirm your changes do not include backports from Mapbox projects (unless with compliant license)
  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>

@HarelM
Copy link
Member Author

HarelM commented Jul 13, 2021

OK, Updated the ci files.
I think circle ci is not in use any more, I think I'll remove it.

The problem is that tap and flow do not work well together with node 14, in terms of importing files without file extension.
Although this works when adding the parameter --experimental-specifier-resolution=node it break the flow removal code, I think, not sure...

Worth noting that tap is far down the bottom of this chart.
https://www.npmtrends.com/ava-vs-jest-vs-karma-vs-mocha-vs-tap

I'm familiar with karma from my angular work, but maybe we can use jest somehow to resolve this mess...
I'll use this branch for my experiments.

@astridx
Copy link
Contributor

astridx commented Jul 14, 2021

The problem is that tap and flow do not work well together with node 14, in terms of importing files without file extension.
Although this works when adding the parameter --experimental-specifier-resolution=node it break the flow removal code, I think, not sure...

Because these tests worked in my last attempt, I just uploaded my play branch. Maybe you would like to have a look.
https://github.com/maplibre/maplibre-gl-js/compare/main...astridx:play?expand=1

I think that tap works with flow. Here you can see that there is a parameter for this: https://node-tap.org/docs/using-with/.

But, in this repo there are special cases. We don't use the general library https://github.com/facebook/flow, but.
https://github.com/mapbox/flow-remove-types.

I tried the --flow parameter with tape, but it didn't work. And I tried to use https://github.com/facebook/flow. It didn't work either.

@HarelM
Copy link
Member Author

HarelM commented Jul 14, 2021

I'm not saying it's not possible, but it's sure as hell not out-of-the-box. I've looked at the solution you found and it felt "too hackish" to me.
After a long thought about this subject I decided the best approach would be to use typescript and jest. See #209.
So if you really want to push this forward it's OK by me, but I think investing our time in #209 would be better.
You can also read about it in the slack channel.
Let me know what you decide.

@wipfli
Copy link
Member

wipfli commented Jul 17, 2021

I will close this pull request and delete the node-14 branch in favor of #209 which actually has cherry-picked the changes from this pull request.

@wipfli wipfli closed this Jul 17, 2021
@wipfli wipfli deleted the node-14 branch July 17, 2021 17:10
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

3 participants