isaacs / tshy

Other
890 stars 18 forks source link

Fallback array in exports as option #28

Closed DaniGuardiola closed 1 year ago

DaniGuardiola commented 1 year ago

This is a weird one. Apparently, if you use an ESM Tailwind CSS config file and try to import from an ESM (maybe commonjs too, haven't tested) package that has conditional imports, it breaks and says Package subpath './<subpath>' is not defined by "exports" in <package.json path>.

Somehow, I've found that this "fallback syntax" works:

{
    "exports": {
      // ...
      "./config": [
        {
          "import": {
            "types": "./dist/esm/tailwind-config.d.ts",
            "default": "./dist/esm/tailwind-config.js"
          }
        },
        "./dist/esm/tailwind-config.js"
      ]
    }
}

So I'd love to be able to have tshy generate this for me, in lack of Tailwind behaving properly.

I think a good, simple way would be to have a fallbackSyntax option, potentially of type boolean | string | string[] where the string/strings indicate the exports that should use the fallback syntax, and if true then all exports do.

Currently, I'm about to build a script that wraps tshy and fixes the package.json afterwards as a workaround, though I don't love it.

DaniGuardiola commented 1 year ago

If anyone's interested, this is how I worked around this issue using zx:

#!/usr/bin/env zx
import { $ } from "zx";
import fs from "node:fs/promises";

async function tweakPackageJson() {
  const path = "./package.json";
  const packageJson = JSON.parse(await fs.readFile(path, "utf-8"));
  const exportContent = packageJson.exports["./config"];
  const jsPath = exportContent.import.default;
  packageJson.exports["./config"] = [packageJson.exports["./config"], jsPath];
  fs.writeFile(path, JSON.stringify(packageJson));
  await $`prettier --write ${path}`;
}

async function build() {
  await $`bun tshy`;
  await tweakPackageJson();
}

await build();
isaacs commented 1 year ago

I can use an array with a string for the default as the last item, since that's functionally equivalent, but I'm wondering if this is maybe a bug in whatever is loading things? Can you share a program that demonstrates this behavior?

isaacs commented 1 year ago

Oh, wait, no, what you have here is not safe. If someone does require('pkg/config') then they'll get the ESM version, that's not what you want.

Definitely need to see this actually fail in order to figure out how to safely address it. Providing the ESM export in the require() condition is not a good idea.

DaniGuardiola commented 1 year ago

I am about to publish the package where I needed this hack. You should be able to then try to import it from a tailwind config file and tweak the package JSON to cause the error and see for yourself. Will update the thread when ready. Thanks for your continued support :)

DaniGuardiola commented 1 year ago

Here's the project: https://github.com/DaniGuardiola/tailwind-ink

Here's what you can do to repro:

bun i bun prepare (will run build.zx.js which runs tshy and then the hack I mentioned earlier) bun link bun link tailwind-ink

This will link the project to itself. It's odd, but it works as a simple repro lol. Now in tailwind.config.js, replace the relative import with the package subpath:

- import { tailwindConfig } from "./dist/esm/intellisense/index.js";
+ import { tailwindConfig } from "tailwind-ink/intellisense";

Now run bun tailwindcss -o /tmp/test. Check out the package.json exports (the array hack is applied).

It will work without errors. (No utility classes were detected is fine, not an error)

Now to break, it, comment out line 20 in build.zx.js (await tweakPackageJson();)

bun prepare bun tailwindcss -o /tmp/test

Now it breaks:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './intellisense' is not defined by "exports" in /home/dani/projects/tailwind-ink/package.json imported from /home/dani/projects/tailwind-ink/tailwind.config.js
    at exportsNotFound (node:internal/modules/esm/resolve:294:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:584:13)
    at trySelf (node:internal/modules/cjs/loader:558:34)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1121:24)
    at Function.resolve (node:internal/modules/helpers:187:19)
    at _resolve (/home/dani/projects/tailwind-ink/node_modules/jiti/dist/jiti.js:1:251148)
    at jiti (/home/dani/projects/tailwind-ink/node_modules/jiti/dist/jiti.js:1:253746)
    at /home/dani/projects/tailwind-ink/tailwind.config.js:1:156
    at evalModule (/home/dani/projects/tailwind-ink/node_modules/jiti/dist/jiti.js:1:256443)
    at jiti (/home/dani/projects/tailwind-ink/node_modules/jiti/dist/jiti.js:1:254371) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}
error: "tailwindcss" exited with code 1 (SIGHUP)

Let me know if you manage to repro :)

DaniGuardiola commented 1 year ago

FYI the package used in the repro above was renamed to classy-ink so if you try it just replace it in all steps.

isaacs commented 1 year ago

This is happening because node_modules/jiti/dist/jiti.js calls require.resolve() with that path.

I'm not very familiar with the internals of tailwindcss, but it seems like it's just not fully compatible with ESM, it's assuming that the module can be resolved via require.resolve(), which is commonjs-only.

Note: the approach here seems to make it work, because it doesn't actually load the module through require, only resolve them using the require.resolve method. But having that opening in your module is fundamentally incorrect. It means that the module can be resolved with require.resolve, but if you were to try to require() it, it'll fail.

> require.resolve('classy-ink/intellisense')
'/Users/isaacs/dev/isaacs/classy-ink/dist/esm/intellisense/index.js'
> require('classy-ink/intellisense')
Uncaught:
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/isaacs/dev/isaacs/classy-ink/dist/esm/intellisense/index.js not supported.

What tshy is building is correctly setting require.resolve() and require() to match each other:

$ tshy
$ node
Welcome to Node.js v20.8.0.
Type ".help" for more information.
> require.resolve('classy-ink/intellisense')
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './intellisense' is not defined by "exports" in /Users/isaacs/dev/isaacs/classy-ink/package.json imported from /Users/isaacs/dev/isaacs/classy-ink/
> require('classy-ink/intellisense')
Uncaught:
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './intellisense' is not defined by "exports" in /Users/isaacs/dev/isaacs/classy-ink/package.json imported from /Users/isaacs/dev/isaacs/classy-ink/
isaacs commented 1 year ago

Note the warning here: https://arethetypeswrong.github.io/?p=classy-ink%401.1.0

DaniGuardiola commented 1 year ago

Hmm thanks for looking into this. So I guess the tailwind export needs to be cjs? The thing is, the main module needs to be ESM only because of Ink, which is ESM only. Is there a way in tshy to configure ESM/cjs by export?

isaacs commented 1 year ago

Is there a way in tshy to configure ESM/cjs by export?

yup! Anything named .cts will be commonjs only, anything named .mts will be esm only.

DaniGuardiola commented 1 year ago

Hi @isaacs, I'm trying to do this now but I'm actually not clear on how to do it.

Here's the relevant tshy config: https://github.com/DaniGuardiola/classy-ink/blob/main/package.json#L61

I've tried to do:

-"./intellisense": "./src/intellisense/index.ts"
+"./intellisense": "./src/intellisense/index.cts"

But tshy ignores that export entirely and renaming that file also causes issues with all imports: "The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("./.js")' call instead."

So I'm not sure how to pull this off?

isaacs commented 1 year ago

Oh, well, yeah, you can't import ink into a cjs file with require(). It's ESM-only. I thought that was only cjs.

So you're in a pickle here. You want to use ink, and also tailwind. Ink is esm-only, and tailwind only resolves modules with require.resolve.

The real solution (since tailwind does actually load with import(), not require()) is that tailwind should be using something like http://npm.im/resolve-import instead of require.resolve. That's a bug. They're doing it wrong, and forcing you to do it wrong as a result.

The less good solution is to lie in your own exports, which is what you were doing. Anyone who tries to require() that ./intellisense entry point is in for a surprise, though. Never gonna work.

So you have a situation where you have to pretend that something is a require() endpoint, when in fact it's strictly ESM. Submit an issue to tailwind complaining about this, and put a caveat in your docs that the types are broken for that reason, I guess?

DaniGuardiola commented 1 year ago

@isaacs Interestingly, the import error happens even for files that definitely do NOT depend on Ink, which is strange to me. And also there's the fact that for the tailwind export, ink is only imported for its types, and no runtime artifact makes it to the final build if you use that entry point. So I feel like I'm missing something here :/

In any case, this is actually a hack for the most part, tailwind is actually never used in the first place, the only important part is that their Intellisense plugin loads it to have IDE support for my package. So a little lie doesn't actually hurt. I might still file a bug, but in different circumstances where this actually needs to run, I would be left questioning if CommonJS can be supported at all just for that endpoint.

Thanks for your help throughout this!

isaacs commented 1 year ago

Interestingly, the import error happens even for files that definitely do NOT depend on Ink,

I'm seeing it error in the following places when I remove "dialects": ["esm"] and rename src/intellisense/index.ts to .cts:

Screenshot 2023-11-16 at 13 17 53

When the commonjs dialect is built, any *.ts files are interpreted as CommonJS, so they fail when they load anything from an ESM-only package.

If I just do the file/export rename, but don't change dialects, then the build succeeds, but of course, then the ./intellisense entry point isn't built, so require.resolve() will still fail on it.

I might still file a bug, but in different circumstances where this actually needs to run, I would be left questioning if CommonJS can be supported at all just for that endpoint.

It certainly can. But it means that in the CommonJS build, you would have to load any ESM-only packages (such as ink or chalk) with the async import() method, not an import statement. This can be very inconvenient, basically you have to make the entire lib async and lazy-load all dependencies.

isaacs commented 1 year ago

If you do file a bug, I'd recommend removing your xz hack, and report that your properly-exported ESM-only entrypoint module is not being picked up, and suggest that they use resolve-import instead of (or along with) require.resolve, since they do not limit their actual loading to only modules that are compatible with require().