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: convert HTMLNano to Es Modules #260

Merged
merged 14 commits into from Oct 16, 2023
Merged

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Oct 3, 2023

Fixes #259

This removes all the usage of commonjs from HTMLNano replacing it with standard ES modules. The optional dependencies are now imported using dynamic imports. So, the functionality will be preserved.

All the current tests pass. I added commonjs compatibility tests to make sure nothing breaks.

The minify function returned by htmlnano is now async due to the dynamic import. posthtml should await this promise. Based on the tests, posthtml already does this, so there should not be an issue.

This change allows adoption of HTMLNano to new projects that only work with pure ES modules.

Tests passing:
image

Backwards compatibility tests:
image

Copy link
Contributor

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

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

I am not a fan of pure ESM package, as well as the existing community. htmlnano is still used by toolchain/bundler that uses CommonJS. Would you like to make it a dual ESM/CJS?

@aminya
Copy link
Contributor Author

aminya commented Oct 3, 2023

I am not a fan of pure ESM package, as well as the existing community. htmlnano is still used by toolchain/bundler that uses CommonJS. Would you like to make it a dual ESM/CJS?

If you notice, I have kept the package backwards compatible. The main entry still points to the .cjs file compiled by Babel from the ES module.

In fact, I added some commonjs compatibility tests for that:
https://github.com/aminya/htmlnano/blob/es-modules/test/htmlnano_compat.cjs

@aminya
Copy link
Contributor Author

aminya commented Oct 7, 2023

Could you approve the CI?

@maltsev
Copy link
Member

maltsev commented Oct 7, 2023

Could you approve the CI?

Sure. Just did.

@aminya
Copy link
Contributor Author

aminya commented Oct 8, 2023

Thank you. This PR is ready. Let me know if there is anything else to be done here.

@maltsev
Copy link
Member

maltsev commented Oct 8, 2023

@SukkaW what do you think, can it be merged now?

@aminya
Copy link
Contributor Author

aminya commented Oct 15, 2023

This pull request is ready, but I made some changes to the Babel plugin responsible for .cjs extension backwards compatibility, making the output more efficient.

silane/babel-plugin-replace-import-extension#9

Depending on the timing of the merges, I'll either update this PR or make a new PR to htmlnano with the improvement.
You can preview the changes on my fork on the main branch:
https://github.com/aminya/htmlnano

@aminya
Copy link
Contributor Author

aminya commented Oct 15, 2023

I published my fork of the Babel plugin to unblock us. Now, the dynamic imports can be processed by bundlers (e.g. for usage as an Astro plugin) and the cjs output is very efficient!

Now, my astro-htmlnano plugin finally works:
https://github.com/aminya/astro-plugins/tree/main/packages/astro-htmlnano

@maltsev maltsev merged commit a231dad into posthtml:master Oct 16, 2023
4 checks passed
@maltsev
Copy link
Member

maltsev commented Oct 16, 2023

Thanks!

@aminya I guess this could be released as a minor version or patch as it doesn't contain any backward incompatible changes?

@aminya
Copy link
Contributor Author

aminya commented Oct 16, 2023

Thanks!

@aminya I guess this could be released as a minor version or patch as it doesn't contain any backward incompatible changes?

You are welcome!

I'd go with a minor version as the addition is a new feature. But it should be backwards compatible.

@maltsev
Copy link
Member

maltsev commented Oct 19, 2023

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.

Do not use dynamic require to load modules
3 participants