-
Notifications
You must be signed in to change notification settings - Fork 13
Upgrade JS package dependencies (yarn upgrade). #90
Conversation
Repro: cd native yarn upgrade --latest yarn remove babylon Several of the packages depended upon by this driver have not been updated in a while. Most importantly, the "babylon" package has been renamed @babel/parser. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
Unfortunately, it looks like I'm going to have to split the Babel upgrade from the rest of this change; there are just too many AST changes and no clear documentation as to what the API guarantees. |
🙏 and there conveniently is a #47 for it |
I surveyed the diffs between the existing native fixtures and those generated by the new version of the parser. As far as I can tell, all of the differences are the addition/removal of one or more of the following properties:
This seems to be a straightforward metadata extension of some kind, and I think it should be safe to update the fixtures on that basis. @dennwc do you think that is reasonable? |
Unfortunately, while splitting that part of the change is appealing for reasons of organization, there is (it turns out) no easy way to cut that one piece out: Upgrading Babel also pulls in a whole bunch of other updates, which is more or less what this change does anyway. And those updates are the cause for the rest of the changes in this PR already. Worse, I found that if I leave the old version pinned in place I can't update the packages I need to update to fix the upstream bug. 🤷🏻♀️ So I think in practice it will not save anything to separate them. |
The only additional change this required was to add an option for the pipeline operator plugin, to specify which of the various implementation strategies is to be preferred. I chose "minimal" as it appears to be the one most similar to the previous default behaviour. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
It has been a while since we updated the tool, and some of the options have moved around. This is as close as I could come to keeping everything the same. I moved some deprecated options to their now preferred home. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
Previously, the fixture to test module parsing referred to an unresolvable import. This now causes the parse to fail by default. Update the code to trigger module behaviour without failing in the resolver. The key property is the presence of import/export statements. At the same time, suppress a complaint from the more recent versions of AVA, that there were no tests in the fixture, by adding a trivial test that always passes. Since the purpose of this fixture is to parse successfully, the ability to execute the test at all means it passed. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
I have updated the fixtures for the output changes described above. The errors that remain are because the parser is now more strict than it used to be, and will complain for invalid structures like re-definitions of existing names, and exports of non-existing names. Our existing parser logic has some custom hooks to "guess" which mode to run the parser in ("module" or "script"). An error in the first falls back to the second. This means when we have an invalid file in module mode, we also get an error from script mode. But since script mode cannot handle newer ES constructs like My plan for now is to make the fixtures "valid", and to address the fallback weirdness in a separate PR. |
Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
Modern Babel versions will report an error if an export declaration refers to an identifier that does not exist. Add declarations for the names exported by the declaration, and update the positions in the golden output. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
Modern Babel versions will report an error if an import declaration tries to re-import an identifier that was already imported. Update conflicting names and the corresponding test output locations. Signed-off-by: M. J. Fromberger <michael.j.fromberger@gmail.com>
I believe this is now ready for a review. While a lot of files are involved, most of the changes are mechanical. The changes that should be reviewed closely are:
The commits in this PR separate these changes as much as is practical, so you may find it easier to review commit-by-commit. I will do my best to keep that structure during the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding fields may break existing annotations though. CI is green which is a good sight, but let me review all the fixtures just in case. For example, the disappearance of leadingComments
is worrying - those are used in our annotations. But they might be optional.
Reviewed 5 of 5 files at r2, 72 of 381 files at r3.
Reviewable status: 77 of 386 files reviewed, all discussions resolved (waiting on @bzz and @dennwc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
,leadingComments
looks safe, we drop it on our side anyway.
Also, I expected something more disruptive in this update. They mostly wiped empty fields and added a few new ones. Also, I've noticed they now differentiate between "module" and "script" file types better.
It may make sense to add fixtures that cover the new fields later. Looking at the diff, all those fields are empty, so we are missing some exotic fixtures, it seems.
Reviewed 309 of 381 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bzz)
This change is