kwhitley / itty-router

A little router.
MIT License
1.7k stars 77 forks source link

itty-router default import switch from CJS to ESM #170

Closed dannyrb closed 1 year ago

dannyrb commented 1 year ago

I encountered this issue when upgrading:

From: itty-router@^3.0.10 To: itty-router@^4.0.9

I see the following when attempting to run my jest test suite:

SyntaxError: Unexpected token 'export'

It looks like at some point between those versions the default export changed from CJS to ESM? If I navigate to here, I see that both a CJS and ESM version are published. Changing how I import itty-router appears to resolve the issue.

From:

import { IRequest, Router, RouterType } from "itty-router";

To:

import { IRequest, Router, RouterType } from "itty-router/cjs";

If this is a part of the v3 -> v4 migration, I recommend updating the migration guide or adding a small note. Maybe a small snippet of guidance for folks hoping to use this with test runners?

dannyrb commented 1 year ago

😅 Me again

The above caused issues with wrangler dev for me. I've instead opted to keep the import as is and update the jest config:

    "transformIgnorePatterns": [
        "node_modules/(?!itty-router)"
    ],
kwhitley commented 1 year ago

Hmmm - thanks for the heads up! Taking a look now!

I had huge headaches trying to get both ESM and CJS to detect appropriately with named exports in the package.json... eventually giving up and opting for an ESM-first strat. Definitely should have noted that in the migration docs though!

kwhitley commented 1 year ago

Hey @dannyrb - I've just released an update to fix this issue (and no longer require the /cjs prefix)... mind updating to 4.0.10 and seeing how that works in your setup?

kwhitley commented 1 year ago

Unfortunately, despite having fixed the exports, in order to remain fully backwards compatible for this major version, I have to leave things in the cjs folder (under the hood), complete with their own .d.ts files... next major release, I'll definitely clean this up to reduce the total files exported (all the duplicate .d.ts files) by putting the CJS files alongside the ESM ones as .cjs.js.

Sigh.

darsovit commented 1 year ago

With 4.0.11, we're failing to be able to run our test code with itty-router (with either vitest or jest), so we've backed up to 4.0.9 for now.

I'm copying this in from discord:

I did find this in the vitest issues: https://github.com/vitest-dev/vitest/issues/1387 It provides a workaround I can do in the meantime with itty-router, specifying in vitest.config.js's defaultConfig({ test: { deps: inline: ['itty-router'] } })

But I'll note, there is a response from sheremet-va indicating that 'vitest imports a cjs version of the package wagmi because their exports field is incorrect', with an explanation linking to: https://github.com/sheremet-va/dual-packaging -- I'll just add this same detail to the 170 issue, since this seems to be where things got broken.

kwhitley commented 1 year ago

@darsovit this is super helpful, thanks! I'm looking into this issue right now, as different combinations of environments suddenly keep messing with it...

kwhitley commented 1 year ago

@darsovit - could you try with itty-router@next (should be 4.0.11-next.7)?

kwhitley commented 1 year ago

If that works, #177 is pending to release as latest with the fix that hopefully resolves (I feel like there's a pun in there...) this once and for all.

darsovit commented 1 year ago

Yes, this works for my unit tests; my sample repo as well as real repos using both vitest and jest where they were failing before.

Thanks, Jonathan

kwhitley commented 1 year ago

Appreciate the checking! Merged and into 4.0.12 :)