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 "types" to package export #7

Closed
wants to merge 2 commits into from
Closed

Add "types" to package export #7

wants to merge 2 commits into from

Conversation

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2022

🦋 Changeset detected

Latest commit: 11fe076

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
zod-validation-error Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@nikoskalogridis nikoskalogridis left a comment

Choose a reason for hiding this comment

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

Thank you

@ivan-kleshnin
Copy link

ivan-kleshnin commented Dec 10, 2022

"module": "path-to-esm-dist" and maybe something else has to be added to unblock ESM-based projects to use this library 🤔 It currently fails at import with:

.../node_modules/zod-validation-error/build/node/esm/index.js:1
export { ValidationError, toValidationError, isValidationError, fromZodError } from './ValidationError';
^^^^^^

@kkirby
Copy link
Author

kkirby commented Dec 10, 2022

@ivan-kleshnin This seems out of scope from this PR. Aside, I went down a rabbit hole looking into your suggestion and this is what I found:

  1. The "module" field isn't actually supported by NodeJS, and instead they settled on using "exports":
    https://stackoverflow.com/a/42817320/701263
  2. It seems most people agree that a package shouldn't duplicate the source code for different module resolutions. Instead, you should target CJS, and then have some kind of wrapper for MJS. I'm not sure how that would work.
  3. If you wanted to still compile it to both, you'd need to do some things:
    1. Update all the .ts files to import their dependencies using .js. The files can remain .ts, but the output files will include the .js extension, which is required by NodeJS when using ESM:
      https://nodejs.org/api/esm.html#mandatory-file-extensions
    2. Provide a package.json file in the build/node/esm and build/node/cjs folder that contain the following:
    {
      "type": "module"
    }
    and
    {
      "type": "commonjs"
    }
    1. That should be it. NodeJS will correctly require and import the module. It's just annoying that you have to import using .js in the .ts files.

For more information, check out this lengthly discussion: microsoft/TypeScript#18442

@ivan-kleshnin
Copy link

ivan-kleshnin commented Dec 12, 2022

@kkirby thanks for the response! It's a very complex topic and I'm far from being an expert there. I can only add the following points:

  1. zod-validation-error is one of very few libraries that don't compile with my TS/ESM setup. So I assume something is not right or not finished here, not on my side. Other dependencies, including zod work fine.
  2. I don't use file extensions as it's possible to omit them in modern TS/ESM, in NodeJS at least. Here's a relevant part of my consumer-side tsconfig for NodeJS app:
{
  "extends": "@tsconfig/node18/tsconfig.json",
  "compilerOptions": {
    "lib": ["esnext", "dom"],
    "baseUrl": ".",
    "module": "esnext",
    "target": "esnext",
    "moduleResolution": "node",
    "esModuleInterop": true
  },
  "ts-node": {
    "esm": true,
    "experimentalSpecifierResolution": "node"
  }
}

Imports look like:

import conf from "common/config" // not common/config.js
import {makeLogger} from "common/logger" // not common/logger/index.js

As for module... I remember how NodeJS team wanted to enforce their "genius" 🤡 .mjs idea and got a pushbask so hard they had to provide an alternative. So they can invent whatever they want, in reality hovewer I see type: "module" everywhere, unlike mjs, and I see workspaces everywhere, unlike imports: #internal, which is mentioned only in the docs. There's a growing list of NodeJS competitors: we already have Deno, Bun and a couple of other minor contenders. NodeJS is not a monopoly anymore, so they better listen to the audience and adapt, if they want to survive.

@dimitrisnl
Copy link
Contributor

dimitrisnl commented Dec 12, 2022

Hey, @ivan-kleshnin I want to take a closer look at this problem. Would you mind opening an issue? Let's move the discussion there so we can unblock this PR.

edit: I have released a beta version. I would appreciate some feedback to ensure we're on the right track https://www.npmjs.com/package/zod-validation-error/v/0.2.2-beta.1

@ivan-kleshnin
Copy link

ivan-kleshnin commented Dec 12, 2022

Hey, @ivan-kleshnin I want to take a closer look at this problem. Would you mind opening an issue? Let's move the discussion there so we can unblock this PR.

Sure, I'll open a dedicated issue soon. Thank you!

@dimitrisnl
Copy link
Contributor

dimitrisnl commented Dec 14, 2022

Hey, I opened a new PR that will, unfortunately, make this one obsolete.

I apologize for this but thank you for bringing these issues to my attention. We've decided to drop swc and the exports key. WIll revisit it in the future, but it seems that ESM works properly now.

@dimitrisnl dimitrisnl closed this Dec 14, 2022
@kkirby kkirby deleted the patch-1 branch December 14, 2022 13:12
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.

4 participants