Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

guillaumervls
Copy link

Followup of #129 (comment)

@guillaumervls guillaumervls mentioned this pull request Sep 15, 2021
5 tasks
Copy link
Member

@benjie benjie left a 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...

src/commands/_common.ts Outdated Show resolved Hide resolved
@benjie benjie changed the title Add support for .gmrc as an ES Module feat(esm): add support for .gmrc as an ES Module Sep 16, 2021
@benjie
Copy link
Member

benjie commented Sep 23, 2021

You know what... Let's just drop Node 10 support. Node 12 should handle this natively and 10 is no longer LTS anyway.

@benjie
Copy link
Member

benjie commented Sep 23, 2021

@guillaumervls Please could you test to see if this works in your setup? I anticipate it will.

@guillaumervls
Copy link
Author

@benjie Yes it works! (also I've fixed the prettier fail)

Copy link
Member

@benjie benjie left a 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 👍

@andrew-w-ross
Copy link
Contributor

@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.

@benjie
Copy link
Member

benjie commented Jul 14, 2022

Just tested this on Node 16 and it breaks commonJS compatibility 😞

@benjie
Copy link
Member

benjie commented Jul 14, 2022

It turns out this doesn't use the native import due to TypeScript configuration.

Copy link
Member

@benjie benjie left a 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.

@benjie
Copy link
Member

benjie commented Jul 14, 2022

@andrew-w-ross Feel free to raise a PR against this branch to fix the remaining issues.

@andrew-w-ross
Copy link
Contributor

andrew-w-ross commented Jul 15, 2022

@benjie I gave it a go this evening and to actually fix it I'd have to change typescript module mode to Node16 or NodeNext to get typescript to emit dynamic imports. That seemed like a step too far so instead I went for a safer route that allows .cjs extension on config files #161. It at least gives a workaround for es module packages.

@andrew-w-ross
Copy link
Contributor

@benjie Have you had a chance to look at that pr I opened?

@benjie
Copy link
Member

benjie commented Nov 8, 2023

(The above mentioned PR was #161 for anyone looking.)

graphile-config is Graphile's new configuration format to be shared across all the Graphile projects, and it supports ESM, CJS, TypeScript and more. We will probably move away from a .gmrc file to a graphile.config.ts (or similar) file for Graphile Migrate, which will address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants