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

Support ECMAScript modules. #8

Merged
merged 7 commits into from
Aug 30, 2023
Merged

Conversation

BowlingX
Copy link
Contributor

@BowlingX BowlingX commented Apr 20, 2023

This PR adds support for es modules. This fixes #4.

I further bumped zod and added missing cases that where not handled currently.
I wanted to update typescript to version 5, but it's causing one type error (on this line: https://github.com/BowlingX/znv/blob/6b69a204a35dd6bf86e345e12a22ee27210a31b4/src/parse-env.test.ts#L305-L305).

I was not able to fix it.

@BowlingX BowlingX changed the title Support es modules. Support ECMAScript modules. Apr 20, 2023
@BowlingX
Copy link
Contributor Author

I published the package as [email protected] for testing.

@lostfictions
Copy link
Owner

lostfictions commented Apr 20, 2023

Thanks a lot for this! I'll take a closer look in a few days, but it looks pretty good to me at a glance so far. One comment is that generally I'd like to try to restrict both zod and typescript to the minimum workable versions to ensure that things keep working for consumers of this package on upgrades.

It's true that znv doesn't express a peerDependency on typescript, but if we bump it and that somehow causes us to start emitting declarations that aren't backwards-compatible, that's an issue. As far as I've seen those sorts of breaking changes are rare, but it's something I'd prefer to be careful about.

Similarly, I'd prefer not to bump zod, especially if the only change is to add more incompatible types -- in this scenario I think we'd be better off amending the default case in getPreprocessorByZodType to handle forwards-compatibility with a more graceful error message. (Eventually when we do bump our devDependency on zod, the peerDependency in package.json should be bumped to match to make sure consumers don't stay on an old version and hit errors.)

For now, would you be willing to remove the zod version bump? I'd be happy to discuss them in a separate issue or PR, but I'd like to limit this PR to only changes related to ESM support if possible. The typescript bump can stay as-is for now since it seems they're still ironing out edge cases with ESM in new versions.

@BowlingX
Copy link
Contributor Author

BowlingX commented Apr 20, 2023

Thank you very much for this library :).

The lowest working version of zod with es modules is 3.13.2. Before they did not properly specify the types field inside the exports entry inside the package.json (see https://github.com/colinhacks/zod/releases/tag/v3.13.2).

Version 3.13.2 is over a year old already, I would think this is a large enough time for backward compatibility, but of course, I don't know all the consumers of the library.

I adjusted the PR to use this version instead.

@BowlingX
Copy link
Contributor Author

@lostfictions, did you already have time to look at the PR?

@kotx
Copy link

kotx commented Jun 29, 2023

@lostfictions I use "type": "module" in my package.json and would like this to be merged! Please take a look if able.

Copy link
Owner

@lostfictions lostfictions left a comment

Choose a reason for hiding this comment

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

Thanks again for this PR and for making the additional changes I requested!

And thanks for your patience -- I know this is a long time to wait for a review. I'm no longer at an employer that is okay with me taking time out to contribute to open source, so this is all unpaid work on my own time (and I haven't had much free time for the past little while).

Overall this looks good to me, though I haven't cloned it and tried it yet. I've left a few comments and suggestions -- let me know what you think about them.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/reporter.ts Outdated Show resolved Hide resolved
tsconfig.cjs.build.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
esm-postbuild.sh Outdated
Comment on lines 1 to 5
cat >dist/package.json <<!EOF
{
"type": "commonjs"
}
!EOF
Copy link
Owner

Choose a reason for hiding this comment

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

oof, it seems unfortunate that an hackish little script like this is necessary to support dual-format packages. there's really no simpler way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know of any other way actually. An alternative is to use different extensions (e.g. .cjs), but this would be more complicated to setup. We can also inline the script in the package.json if you prefer this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative could be to just commit the package.json into the repository (inside a dist folder)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed another version with the simplified build

Copy link
Owner

@lostfictions lostfictions Aug 30, 2023

Choose a reason for hiding this comment

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

good idea! for simplicity i think i'll move the additional package.json to the project root under a different name (rather than unignoring it and using find). but i'll make that change after merging rather than requesting more changes from you!

@lostfictions
Copy link
Owner

looks great! thanks so much for all your work on this, and sorry again for the delay. i'll cut a new release shortly!

@lostfictions lostfictions merged commit e770842 into lostfictions:master Aug 30, 2023
lostfictions added a commit that referenced this pull request Aug 30, 2023
@lostfictions
Copy link
Owner

published as [email protected]!

@BowlingX
Copy link
Contributor Author

Awesome! Thank you!!

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.

Vite's environment variables
3 participants