-
Notifications
You must be signed in to change notification settings - Fork 148
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
lookup: add babel #628
lookup: add babel #628
Conversation
Does babel have any equivalent |
Actually no, the
|
Maybe that would be simplest and would work with #696 in its current form. Alternative options include expanding #696 so that instead of being limited to |
This comment has been minimized.
This comment has been minimized.
It seems like we cannot make it work without babel being a git repository. https://github.com/targos/citgm/runs/2046109107?check_suite_focus=true
|
I can fix it in our scripts. |
lib/lookup.json
Outdated
"maintainers": "nicolo-ribaudo", | ||
"prefix": "v", | ||
"yarn": true, | ||
"scripts": ["bootstrap", "build", "test"] |
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.
build
shouldn't be needed, it's already done by bootstrap
.
I think the main issue is the |
Actually, it seems
|
New run with the |
Should we skip on Node.js 10.x ? |
We test Babel on Node.js 10 so it should work without problems (there are a few disabled tests that rely on native ESM). However, we'll drop Node.js 10 support in a few months. |
Well, it failed on Node.js 10 because of ESM tests. I don't know why they aren't properly disabled but I think we can safely skip Node.js 10. It's in maintenance and the few patches that we will still do to it (if any) are probably not going to break Babel. |
CI on all platforms: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/140/console |
@nodejs/citgm I think this is ready to land. But they are not related to babel, as they happen before citgm is even downloaded. |
215174c
to
755bffc
Compare
Hey all, I made a boo boo on main and had to force push. I've rebased this branch and force pushed to make sure that you don't have to do extra work because of my mistake |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #628 +/- ##
==========================================
- Coverage 96.46% 96.44% -0.02%
==========================================
Files 28 28
Lines 2149 2139 -10
==========================================
- Hits 2073 2063 -10
Misses 76 76 ☔ View full report in Codecov by Sentry. |
@nodejs/citgm PTAL |
Finally, awesome, thank you! |
Ping for reviews! |
A babel release came out with the |
https://ci.nodejs.org/job/citgm-smoker-pipeline/255/console Something weird happened:
|
Maybe the file was corrupted while downloading? |
It's not supposed to download |
It's the consequence of two things:
|
Here's the relevant part of the code: Lines 145 to 160 in 66069f3
|
Looks like it was broken by https://github.com/npm/npm-package-arg/releases/tag/v10.0.0 and we never realized. |
We have custom code to generate tarball manifest files, maybe it's a problem on our end? |
Possibly. Has anything changed on that side between 7.24.5 and 7.24.6? |
No |
Ok, I think it wasn't erroring before because until my last force-push (with |
Convert `fetchSpec` to "latest" when it's "*" to accomodate for the breaking change that happened in version 10 of the library. Make lookup tests closer to reality by using the package arg parser there and constructing fake module metadata that look more like what the npm API returns. Refs: #628 Refs: https://github.com/npm/npm-package-arg/releases/tag/v10.0.0
I opened #1061 to fix that. |
Convert `fetchSpec` to "latest" when it's "*" to accomodate for the breaking change that happened in version 10 of the library. Make lookup tests closer to reality by using the package arg parser there and constructing fake module metadata that look more like what the npm API returns. Refs: #628 Refs: https://github.com/npm/npm-package-arg/releases/tag/v10.0.0
The only failures are infra-related. I think this can finally land @nodejs/citgm |
Woohoo thanks @targos for taking this to the finishing line! |
Work in progress because babel is still not compatible with citgm. This is what you have to do now to test it: