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

Use .js extension for config/assets.js #122

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

timriley
Copy link
Member

@timriley timriley commented Nov 2, 2023

If we set "type": "module" in our package.json (opting into ES Module-style JavaScript by default), then we can use an ordinary .js extension for config/assets.js.

This feels much more conventional, less surprising to the casual user.

Opting into ES Modules makes Hanami's default assets setup more future-focused, which I think this is a good posture for us. Getting this detail right now is important, since changing this later will be costly and involve manual user changes.

Those people needing needing legacy CommonJS-style JavaScript can still use it by giving their files the .cjs extension (or even taking away the "type": "module" declaration, and at that point, they likely know what they're doing).

As noted by @KonnorRogers, esbuild can also help with CJS/ESM interop, too.

@timriley timriley requested a review from jodosha November 2, 2023 23:28
@timriley timriley added this to the v2.1.0 milestone Nov 2, 2023
@timriley timriley self-assigned this Nov 2, 2023
@timriley timriley force-pushed the use-js-extension-for-config-assets-file branch from 108d6a5 to a0df541 Compare November 2, 2023 23:30
It turns out the .mjs wasn’t necessary for this to work correclty with our standard esbuild config.
@timriley timriley force-pushed the use-js-extension-for-config-assets-file branch from a0df541 to 0979681 Compare November 3, 2023 01:21
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timriley @KonnorRogers I'm annoyed by the .mjs extension as well.

Question: if we force the type: "module" in package.json are we getting in the way of developers that may want/need to declare a different type?

@KonnorRogers, you mentioned a potential conflict with Jest. Anything else that we should be aware of?


Are we just embellishing (.mjs -> .js) but creating potential obstacles?

@KonnorRogers
Copy link

Nothing else I'm aware of. Most new packages all support ESM. Even newer versions of Jest support ESM.

Question: if we force the type: "module" in package.json are we getting in the way of developers that may want/need to declare a different type?

They can always override if needed. As a whole for new projects, its a pretty benign property. It's mostly legacy projects that have a hard time with it that have outdated dependencies (pretty much anything >4yrs old)

@timriley
Copy link
Member Author

timriley commented Nov 7, 2023

Thanks @KonnorRogers :)

They can always override if needed. As a whole for new projects, it's a pretty benign property. It's mostly legacy projects that have a hard time with it that have outdated dependencies (pretty much anything >4yrs old)

Given this, I'm happy for us to proceed here. People who need to do something different to this will know what they're doing (and in addition, if we find there's sufficient demand, we can make sure this is clear via documentation).

@jodosha jodosha self-requested a review November 7, 2023 11:54
@jodosha jodosha merged commit afc20c0 into main Nov 7, 2023
@jodosha jodosha deleted the use-js-extension-for-config-assets-file branch November 7, 2023 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants