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 support #66

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

H01001000
Copy link

@H01001000 H01001000 commented Jan 10, 2024

This PR makes generated files use module imports (with .js ext and refactor import directory to import directory/index), making the generated file be used in an esm environment

close #63

p.s. suggest add eslint or prettier

@H01001000 H01001000 marked this pull request as draft January 11, 2024 09:46
@H01001000 H01001000 marked this pull request as ready for review January 11, 2024 10:19
@Cauen
Copy link
Owner

Cauen commented Jan 11, 2024

Hi @H01001000 Thanks for your contribution

I also added some missing conditions to add the ".js" or "index.js"

p.s. suggest add eslint or prettier

Its already part of the project. I've added to CI checks now.


I was unable to run the example that you have created, its working for you?

image


I also don't quite understand the advantage of this approach (esm) as opposed to the approach that the old example already used. Could you help me clarify this so I can add it to the readme?

@H01001000
Copy link
Author

H01001000 commented Jan 11, 2024

@Cauen can try to use ts-node --esm?

not really are there advantage esm vs cjs (you can read more googling), but if for example if some deps are in esm (e.g. node-fetch) you must use esm instead cjs, otherwise you can't import them node throw error

@Cauen
Copy link
Owner

Cauen commented Jan 11, 2024

@Cauen can try to use ts-node --esm?

Not working but... I think the ideal would be for the "inputs-simple-sqlite-esm" example not to work without the new "esm" support configuration. Exactly as was your case, right?

not really are there advantage esm vs cjs

I'm just confused because I've always used ESM in my projects and I've never had the limitation of inserting ".js" extensions in imports

@H01001000
Copy link
Author

H01001000 commented Jan 11, 2024

@Cauen cuz it works for me

Are you sure you putting "type": module in package.json, and tsconfig compile to es module, without those you are still using commonjs, cuz from what i look tsconfig you use will compile esm to cjs

@Cauen
Copy link
Owner

Cauen commented Jan 11, 2024

Are you sure you putting "type": module in package.json, and tsconfig compile to es module, without those you are still using commonjs, cuz from what i look tsconfig you use will compile esm to cjs

@H01001000 I'm referring to the example you created in the project

@H01001000
Copy link
Author

For that I mean your other project

@Cauen
Copy link
Owner

Cauen commented Jan 11, 2024

As it currently stands, does it work for you?

@H01001000
Copy link
Author

@Cauen see if this work, i suspect the only diff my env and your is im install using yarn

@Cauen
Copy link
Owner

Cauen commented Jan 11, 2024

@H01001000 I'm using Yarn too, even with 4.0 from your change its not working

Same error:

Unknown file extension ".ts" for /home/caue/projects/personal/prisma-generator-pothos-codegen/examples/inputs-simple-sqlite-esm/src/server.ts

@H01001000
Copy link
Author

H01001000 commented Jan 12, 2024

@Cauen could you use node --loader ts-node/esm instead of ts-node
I'm thinking another reason could be I'm still using node 18 instead of 20

@Cauen
Copy link
Owner

Cauen commented Jan 16, 2024

🚀 node --loader ts-node/esm src/server.ts works

But the example project runs even without the need to use this new functionality
Could you update this example to make this new functionality necessary? (As your use case)

I believe you use the build process and use node without ts-node, right?

@H01001000

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.

ESM support?
2 participants