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

Make package ESM by default #4957

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Make package ESM by default #4957

merged 1 commit into from
Jan 9, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Dec 13, 2023

☑️ Resolves

This makes the package using ESM by default (type: "module") and adjusts all scripts used.
Some stuff like stylelint and eslint do not work correctly with ESM yet, so they are kept as commonjs scripts.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@susnux susnux added the 3. to review Waiting for reviews label Dec 13, 2023
@susnux susnux marked this pull request as draft December 13, 2023 23:37
@susnux
Copy link
Contributor Author

susnux commented Dec 13, 2023

Basically this works and simplifies the code base, but it does not fix the Typescript issue

@susnux susnux marked this pull request as ready for review December 22, 2023 12:24
@susnux susnux changed the title Chore/make esm default Make package ESM by default Dec 22, 2023
@susnux
Copy link
Contributor Author

susnux commented Dec 22, 2023

Do we want this for v8 or should I change the target branch to next for v9?

@ShGKme
Copy link
Contributor

ShGKme commented Dec 22, 2023

Do we want this for v8 or should I change the target branch to next for v9?

I think this is fine for v8. It should not affect the distributive and works fine on Server. Probably, we should test on more apps before just in case, e.g., Talk.

@ShGKme
Copy link
Contributor

ShGKme commented Dec 22, 2023

Talk works including calls

@ShGKme
Copy link
Contributor

ShGKme commented Dec 22, 2023

So, do we want to add this to the next minor release?

@skjnldsv
Copy link
Contributor

I think this is fine for v8. It should not affect the distributive and works fine on Server. Probably, we should test on more apps before just in case, e.g., Talk.

Switching package types is a breaking change, even if that does not break on our ecosystem :)
So v9 it is pls

@skjnldsv skjnldsv changed the base branch from master to next December 26, 2023 07:49
@ShGKme
Copy link
Contributor

ShGKme commented Dec 26, 2023

Switching package types is a breaking change, even if that does not break on our ecosystem :)

What does this change break?

It doesn't affect end users of the package. dist is the same, no byte has changed. And we have Node.js v20 engine specified already, so no environment requirements changed. Also, extensions are .cjs and .mjs - it doesn't change module resolution.

So it changes development part, but doesn't change the distributive.

@skjnldsv
Copy link
Contributor

What does this change break?

The module resolution changes.
Pre-processors will now treat this package as module, not saying this will cause issues on our ecosystem, but I'm saying this is no patch release (and arguably, no minor either)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux merged commit dec6175 into next Jan 9, 2024
15 checks passed
@susnux susnux deleted the chore/make-esm-default branch January 9, 2024 14:08
@susnux
Copy link
Contributor Author

susnux commented Jan 9, 2024

Merged into Next for reasons mentioned by @skjnldsv

@susnux susnux mentioned this pull request Jan 23, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants