Update Development Environment - Support Node v14 #190
Conversation
|
Thanks for kicking this off!! |
I see it exactly the same way. I thought about whether I should create the PR at all. I did it because I saw in Mapbox's PR that a lot has been changed to support Node v12+. If someone else can do this in one step, then that's great. Updating to the esm library creates the possibility to proceed in small steps. I do not add the library. We are already using it. I'm just updating it. So far I haven't found any disadvantage. |
|
I've read through the issue related to the mapbox PR and they write there that this didn't work for them. Since I don't think muting failing tests is the right approach, and due to what they wrote it seems that this won't get us to where we want to be, I still think this pr is important to kick off the process, but I don't think we'll be able to easily fix this that way. I might be wrong. Feel free to commit more changes here until this is resolved...? Your call. |
|
Thanks for working on this Astrid. Indeed, we cannot stay with node version 10 forever. Already now, gekodriver |
|
Regarding the typescript discussion, I think there was already a long thread about this at some point. @HarelM do you remember where? |
|
Yes, the discussion is here: #32. |
…d glsl files need improvment
|
Bundle size report: Size Change: -8.85 kB
ℹ️ View Details
|
|
Not sure how the ci works, but you'll need to update the circle ci file with node 14 instead of 10, currently seems like it continues to run everything on 10 so the ci is green... |
|
In my understanding CI is all done with GitHub actions. |
|
You should change the circle-ci. It should be merged as part of this commit. Once this commit lands, the ci should be updated. |
|
Seems like an issue with yarn.lock...? |
|
Now everything is red :(. Is that really correct? Locally, I just started the unit tests. None of them failed. Not even the faulty
I don't know if I can reach my goal with this approach. But the fact that everything is red makes me feel insecure more. |
Hm. I add |
Thank you. After deleting the changes in But all fail again :( |
|
I'm looking at the build which says: You'll get there, don't worry, it'll just take more time... |
|
I'm not a big fan of yarn, I guess I don't understand its added value over npm... |
|
Now all tests are green here on Github. But on my local computer, at least |
|
I closed it in favor of this one... Good work! |
|
I don't know why they are not running... |
|
Can you undo the change of the addition of ".js" extension to all the files? There must be a way without it...? |
I'll look at the two failing scripts tomorrow. If I find a solution to anything I notice, I'll change the draft status.
What disadvantage do you see with the added file extensions? Apart from the work that has now been done, I only see advantages. But maybe I'm missing something? If I see it correctly, the only possibility to do without the file extensions is the use of https://github.com/standard-things/esm. I had started this solution, but run into problems during integration testing that I couldn't find a solution for. The Node documentation says that the extensions are mandatory: https://nodejs.org/api/esm.html#esm_mandatory_file_extensions |
|
Here are the problems the way I see it: |
|
Is it possible to add the extension only to non-flow-typed files? i.e. files that are part of the build and not part of the library? |
You are right, a review is big. About typescript: imports with file extensions work here too, right? I think that adding the extensions is more precise and thus also makes the code easier to read for future developers.
|
If I see it correctly, it is only possible not to use file extensions when importing into node 12+ when using the I could keep trying to get this variant to work. I decided against it because the package was last changed in 2019 and node in its own doc describes the variant with |
|
Don't get me wrong, using ems is not the right solution as it's kinda deprecated. |
This would be great. I can not figure it out but I like to learn.
Nevertheless, just in case, I would find it better if we used the extensions. It is extra work, but the right file is for sure imported. There will probably still be a few js files. But that is my personal opinion. |
|
@HarelM @astridx I'm disappointed to find out this PR directly copied most changes from my PR here mapbox/mapbox-gl-js#10367 (not open-source-licensed), down to small details like the custom loader code. I worked extremely hard to get it working, and copying my work directly is not fair. I urge you to close this PR and start from scratch on your own, without copy-pasting anything from GL JS, otherwise we would have to explore what practices like this mean legally for this project. update: edited for milder language |
|
@marcelnormann @nyurik please discuss this on the steering committee meeting. Note how the checkbox above was checked. |
|
@mourner |
|
@astridx I may have overreacted and apologize for the harsh tone. It's fine to learn and then do similar changes independently (no objections to this at all), but definitely not OK to directly copy proprietary code. |
|
Thanks for the input @mourner. |
|
@HarelM for the record, leaving a few comparisons to demonstrate that some parts were copied as is, not just similar:
|
|
@astridx I now realize this was likely unintentional, so once again please forgive me for the initial angry reaction, and feel free to ask questions if you get stuck when redoing this, and I'll be happy to help. It should also be a lot much easier and faster to do it from scratch cleanly now after everything you've learned. Also, feel free to consult the list of gotchas I left in my PR description (but not the code changes). And you don't have to do the |
|
Thanks @mourner for your kind words and the offer for an olive branch. |
|
this calls for a github action or a bot that can cross checks PRs-vs-PRs for changes similarity via some plagiarism checker, not sure how many false positives can arise, but at least something that can post a warning comment to an open PR linking to PR on mapbox side would be handy surprised there is nothing like that out there |
|
@ambientlight I generally agree this is the right approach. It'd be extremely cool if you could push this forward. |
Please see #129. I already spent some time for research on that issue, but right now there is no real progress. Please let me know if you have some hints for us. |
As a steering committe it is up to us to apologize! Backporting of non-compliant code is unacceptable and against the rules of this project. We will take additional measures to ensure other peoples copyright. |
If I'm right, we don't fully support Node v12+ yet because of the way we integrate ECMAScript modules.
There are two options since Node v12+: : Via library or via flag. As far as I can see, we use the library esm.
By the way: Mapbox changed the way to use ECMAScript modules (esm) in PR 10367. They do not use esm any more.
I have no experience with esm. The last version is almost 2 years old. Nevertheless, I suggest updating to this one, because this way our Node scipts are usable under Node v12+. Not everything runs smoothly with this PR. For example, many tests fail. But maybe so we can better work on supporting Node v12+.I decided to use "type": "module" in main package.json because it is more future-proof.
For this reason, I have had to change
Node.js requiretoES6 import/exportin many places. In addition, the specification of file extensions is mandatory.Launch Checklist