-
-
Notifications
You must be signed in to change notification settings - Fork 57
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(esm): add support for .gmrc as an ES Module #140
base: main
Are you sure you want to change the base?
Conversation
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.
I think this is fine...
You know what... Let's just drop Node 10 support. Node 12 should handle this natively and 10 is no longer LTS anyway. |
@guillaumervls Please could you test to see if this works in your setup? I anticipate it will. |
@benjie Yes it works! (also I've fixed the prettier fail) |
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.
Looks good to me. This will need more than a patch bump and we'll have to officially remove Node 10 support, but I'll do that in a follow-up PR.
I'm working on PostGraphile this week and this does not seem urgent so I'm going to delay merging it, but it's approved ready to go 👍
@benjie Any chance this pr will ever get merged in? It's almost been a year and node 12 has reached eol let alone Node 10. |
Just tested this on Node 16 and it breaks commonJS compatibility 😞 |
It turns out this doesn't use the native import due to TypeScript configuration. |
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.
I've pushed up a commit that tries to do the right thing the first time by hinting CJS or MJS; however the await import(relativePath)
calls are being rewritten by TypeScript to await Promise.resolve().then(() => require(relativePath))
- i.e. they're not using ESM at all.
I wonder if anyone tried to run the previous version of this PR, my testing suggested that it doesn't work with CJS, and the rewriting of the import()
implies it also wouldn't have worked with ESM?
Someone needs to tell TypeScript to not rewrite the await import(...)
syntax, then I think this is good to go.
@andrew-w-ross Feel free to raise a PR against this branch to fix the remaining issues. |
@benjie I gave it a go this evening and to actually fix it I'd have to change typescript module mode to |
@benjie Have you had a chance to look at that pr I opened? |
(The above mentioned PR was #161 for anyone looking.)
|
Followup of #129 (comment)