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

Add importsExtension to the config #599

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

jpholtmeyer
Copy link
Contributor

Added a boolean configuration parameter named esmImports.

If set to true, a .js extension will be added to imports.

If set to false, no extension will be added. (this is the current behavior)

I was using this NPM package to solve my problem:
https://www.npmjs.com/package/fix-esm-import-path

Having the logic integrated into Kanel is more developer friendly for me.

@kristiandupont
Copy link
Owner

Nice, thank you!
I wonder if this covers everything though. I believe that you are allowed to use .ts for Typescript files? Also, should this support c and m prefixes? I have been meaning to dig into all of this for a long time but never got around to it, so I am really asking here, I don't know what the conventions/supported options are.

@jpholtmeyer
Copy link
Contributor Author

Thanks for responding to the PR so quickly.

The source files would use the .ts extension. This would be converted to a .js extension at compile time. I don't think that ESM imports should use the .ts extension.

Since .ts files are compiled to .js files, in most cases, I don't think this would be an issue.

I'm still learning when to use .mjs and .cjs extensions, but this is how I understand it. The .mjs extension identifies a file as an ESM module. The .cjs extension identifies a file a CommonJS module

The Kanel project appears to be using ESM import statements (import / export) rather than CommonJS imports (require / module.exports). So I would say that using either the .js or .mjs would be most appropriate.

The .mjs extension is more explicit in identifying the file as an ESM module. While .js extension uses the settings context from the tsconfig.json and package.json.

While developing (before compilation to JavaScript), it appears that TypeScript is smart enough to associate a reference with an extensionless and .js filename. If a .mjs extension is used before compilation, I get a Cannot find module... error. I'm not sure if this is due to my project settings or not.

To make the setting a little clearer, it could be renamed to something like useJsExtensions instead of esmImports. I think this name change conveys the intent a little better. Also doesn't get us caught up in the semantics of what an ESM import really is.

I'm open to hearing any suggestions and thoughts you might have.

@kristiandupont
Copy link
Owner

Right, maybe the extension should be configurable, then? It seems like the kind of thing that might come up. Otherwise, if you can make the linter and tests happy, I will merge and publish this!

@jpholtmeyer jpholtmeyer changed the title Add esmImports to the config Add importsExtension to the config Sep 13, 2024
@jpholtmeyer
Copy link
Contributor Author

I took your advice and renamed esmImports to importsExtension.
This is a little easier to understand and gives more flexibility for configuration.

The property is an empty string by default and will generate imports the same as before if set to this.

The possible extensions are .ts, .js, .mjs, and .cjs.

The extension will only be applied for project file references (any import path containing a ./).

External package imports will not use the configured extension, if set.
So Zod, Knex, and Kysely imports will not be affected.

I also added some docs and regenerated them.
Not sure how you normally handle that but thought I would regenerate since there was a doc update.

@kristiandupont
Copy link
Owner

Very nice! Just one question: the example config has the extension set to empty string, but the generated models seem to have ".js". Is that a mistake or did I misunderstand?

@jpholtmeyer
Copy link
Contributor Author

That was my bad. I was testing with the extensions and forgot to rerun the example.
Just pushed another commit to revert examples back to their original form (no extension).

@kristiandupont kristiandupont merged commit e73b72d into kristiandupont:main Sep 18, 2024
1 check passed
@kristiandupont
Copy link
Owner

This is now published.

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.

2 participants