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 option to generate ESM exports instead of CJS (#1523) #1861

Merged
merged 3 commits into from
Dec 22, 2021

Conversation

rehanvdm
Copy link
Contributor

@rehanvdm rehanvdm commented Dec 19, 2021

What issue does this pull request resolve?
Adds an option so that the generated code can do both CJS and ESM exports. #1523

What changes did you make?
Adde an extra flag code.exportEsm to generate code with ESM exports instead of CJS.

Is there anything that requires more attention while reviewing?
I wanted to use the code.es5 flag and then make the assumption that ES5 code uses CJS and ES6 uses ESM. But this would have been a breaking change because by default the code is exporting as ES6. So I thought it best to add an additional flag called code.exportEsm. It defaults to false so it introduces no breaking changes.

CJS (default) creates exports as module.exports = for single exports and exports. or exports[ for multiple exports. ESM creates exports as export const and export default for single exports, where the export const has been given a fixed name of validate. This is just to cater for both import methods. Then for the multiple exports, the export default option is not available(or has not been implemented) and must be done with named exports as in export const.

I tested both scenarios for when the code has a single and multiple exports. Note that ESM can not export with special characters in the name, CJS can because it then uses the array syntax like exports[https://example.com/string.json]. I didn't just want it to fail the ESM version when the CJS passed so instead I used some regex to replace special characters and then the ESM export would look like export const https___example_com_string_json

I also needed to add a new devDependencies called module-from-string to be able to test the generated code. It is similar to require-from-string. The package is almost a year old with not a lot of stars or downloads but looks stable and was good enough for the tests. I am not sure how else to test ESM modules, open to suggestions if you do not like this method.

@rehanvdm rehanvdm marked this pull request as ready for review December 19, 2021 20:03
docs/options.md Outdated Show resolved Hide resolved
@epoberezkin
Copy link
Member

This is great, thank you! Please see comments - let me know what you think

@rehanvdm
Copy link
Contributor Author

This is great, thank you! Please see comments - let me know what you think

Okay cool, this will actually be my first Open Source PR. I have done a few things here or there, but this one feels much bigger and certainly the one that took the most time 😅. Great repo by the way, good tests, linting and safeguards. I did struggle a little bit to get it going Windows, but managed with WSL (used to many projects only supporting Linux development)

docs/options.md Outdated Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
lib/core.ts Outdated Show resolved Hide resolved
…r#1523)

- Renamed exportEsm to esm
- Extracted common code
- Changed invalid export names to rather throw an error
@rehanvdm
Copy link
Contributor Author

Changes applied, let me know what you think

@epoberezkin epoberezkin merged commit 418cd0f into ajv-validator:master Dec 22, 2021
@epoberezkin
Copy link
Member

Thank you, this is a very solid piece of work - merged, release is coming.
I would only add named exports to the docs, as you said, and the test when "all exports" actually work with ESM - e.g. when schema IDs are just identifiers, as the spec allows - does it make sense?

@rehanvdm
Copy link
Contributor Author

Pleasure. On the docs and additional PR, I am busy writing/rewriting this https://ajv.js.org/standalone.html doc to have that concrete example I mentioned. I am also including knowledge/the options about ES5/ES6 and CJS/ESM exports + examples.

Might be able to get it done tomorrow night, otherwise only sometime next week (mid-week) - Just for if you want to hold off on the release for this doc change.

@epoberezkin
Copy link
Member

Releases just happen, no problem if it’s after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants