-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: convert to ESM (breaking change) #856
Conversation
this probably won't work with node 14 at all ugh
most of them are now .cjs with the exception of bin/rdme.js
can we move off jest? all my homies hate jest
Update: while I made substantial progress on this, we've hit a pretty massive blocker 😵💫 we rely on Jon's work in #857 should make this transition a lot easier, but until we figure out a different strategy for building an executable (or if EDIT (9/2): Might think about https://deno.com/blog/build-cross-platform-cli Ran into the following error when trying to build an executable with Deno locally:
Deno version info:
I also tried following Node's guidelines for building a single executable in Node v20. I haven't troubleshooted this further but I ran into the following error when following the steps above and trying to run the generated executable:
Here's my { "main": "bin/rdme.js", "output": "sea-prep.blob" } It's weird because running
|
deno needs this
formData.append('spec', { | ||
type: 'application/json', | ||
name: 'openapi.json', | ||
[Symbol.toStringTag]: 'File', | ||
stream() { | ||
return stream; | ||
}, | ||
}); |
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.
@erunion I'd like your blessing with these changes now that we're using formdata-node
— does our API care what we name the file? I just set it to openapi.json
but let me know if you feel otherwise.
also our API mocks inspect this payload so we should be in good shape.
- 14 | ||
- 16 |
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.
with this PR, we're technically requiring Node 18 and above but I'll make that official in a separate PR.
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.
made it look easy
@@ -24,7 +24,14 @@ export default async function streamSpecToRegistry(spec: string) { | |||
|
|||
debug('file and stream created, streaming into form data payload'); | |||
const formData = new FormData(); | |||
formData.append('spec', stream); | |||
formData.append('spec', { |
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.
This work seems fine but formdata-node
is still pretty shitty. Recommend moving off it and node-fetch
as soon as we can. Left a diff in #801 (comment) with the work you can do to this file to move it over to all native APIs.
@@ -220,7 +220,9 @@ async function handleRes(res: Response, rejectOnJsonError = true) { | |||
const contentType = res.headers.get('content-type'); | |||
const extension = mime.extension(contentType); | |||
if (extension === 'json') { | |||
const body = await res.json(); | |||
// TODO: type this better |
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.
Leaving this as any
is probably fine imo. You aren't going to have full documented types for these responses without utilizing our API SDK or anything like that (and even then the types are nonexistent for API v1).
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.
yeah node-fetch
is remarkably bad about typing responses
# [8.7.0-next.3](v8.7.0-next.2...v8.7.0-next.3) (2023-09-14) ### Features * convert to ESM (breaking change) ([#856](#856)) ([84b8571](84b8571)), closes [/www.stefanjudis.com/snippets/how-to-import-json-files-in-es-modules-node-js/#option-1](https://github.com//www.stefanjudis.com/snippets/how-to-import-json-files-in-es-modules-node-js//issues/option-1) [1#L228](https://github.com/1/issues/L228) [skip ci]
🧰 Changes
This PR refactors the entire repository over to full ESM. This entailed a few different refactors:
.js
file extensions for relative imports (really wish I had done this after discoveringeslint-plugin-require-extensions
) 😭ora
, see the outstanding issues below)fetch
so a test or two had to be updated accordingly. Eventually we'll want to migrate away fromnock
entirely.module: NodeNext
in ourtsconfig.json
andtype: module
in ourpackage.json
pkg
😵💫 (see ES modules not supported vercel/pkg#1291)pkg
handles a lot of the madness of compiling into single-file executables better than anything else I've tried (see here for more context)Outstanding tasks/issues
Below is a running list of outstanding work — the
ora
issue is mostly out of our hands, but I'll look into theExperimentalWarning
in a follow-up PR.ora
due to segmentation faults at runtime, see ora@7 dependency causes seg fault (was seg fault on mac m1 arm) sindresorhus/ora#229 for more informationoas
(see fix: get exports working in TSnodeNext
oas#796)oas-normalize
(see fix: get exports working in TSnodeNext
oas-normalize#294)tsconfig
cleanup (e.g., do we needdownlevelIteration
?) (edit: nah let's do this further down the line, we're already introducing a lot of new breaking changes as is).cjs
files over to.js
🧬 QA & Testing
Our existing end-to-end tests GitHub Actions tests are passing, meaning this should work, but I'll do some more QA once we tag a beta release.
You can confirm this all works locally by doing the following: