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

Fix CJS compatibility #490

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix CJS compatibility #490

wants to merge 6 commits into from

Conversation

FurryR
Copy link

@FurryR FurryR commented Aug 14, 2024

Proposed Changes

Ensure compatibility with some CDNs & bundlers on some CJS libraries.

jsDelivr translates const { nanoid } = require('nanoid') directly into import e from 'nanoid/+esm'; const { nanoid } = e, which is problematic and causes some libraries fail to load.

Here is an example using postcss:

await import('https://cdn.jsdelivr.net/npm/postcss@8.4.41/+esm')

Additional Information

It is suggested to squash merge this PR.

@ai
Copy link
Owner

ai commented Aug 14, 2024

  1. We also have url-alphabet
  2. Please run eslint --fix .
  3. I really worry that it could break something else. Can you test that this method doesn’t break Vite and webpack (you can add version from your branch by npm install --save-dev nanoid@FurryR/nanoid

@FurryR
Copy link
Author

FurryR commented Aug 15, 2024

  1. We also have url-alphabet
  2. Please run eslint --fix .
  3. I really worry that it could break something else. Can you test that this method doesn’t break Vite and webpack (you can add version from your branch by npm install --save-dev nanoid@FurryR/nanoid
  1. Sorry that I considered url-alphabet a private file. I will add default export for it later.
  2. okay I will do that later
  3. It seemed that adding a default export is not a breaking change at all, but I am unsure if the type description files (.d.ts) need to be updated. Anyway, I will take a look.

@FurryR FurryR marked this pull request as draft August 15, 2024 04:26
@FurryR FurryR marked this pull request as ready for review September 1, 2024 11:52
Signed-off-by: FurryR <awathefox@gmail.com>
@FurryR
Copy link
Author

FurryR commented Sep 1, 2024

Tested with pnpm start. This PR does not break Vite.

@ai
Copy link
Owner

ai commented Sep 1, 2024

const { nanoid } = require('nanoid')

But Nano ID 4.x and 5.x don’t support require(). If you are using CJS you need 3.x version. We have a special branch for it.

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.

2 participants