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 node 16 cjs import #2112

Merged
merged 11 commits into from
Dec 9, 2024
Merged

Fix node 16 cjs import #2112

merged 11 commits into from
Dec 9, 2024

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Dec 6, 2024

Fixes #2107

From running npm pack locally on PR:

Screenshot 2024-12-06 at 9 24 58 AM

The change is making exports explicit for esm or cjs:

"exports": {
  ".": {
    "import": {
      "types": "./dist/index.d.ts",
      "default": "./dist/index.mjs"
    },
    "require": {
      "types": "./dist/cjs/index.d.cts",
      "default": "./dist/cjs/index.cjs"
    }
  }
}

And generating separate type declarations.

"import": {
"types": "./dist/index.d.ts",
"default": "./dist/index.mjs"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff is noisier than ideal as I ran https://www.npmjs.com/package/fixpack on the package.json

Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

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

Looks good, I guess? 😄 If it passes the checks then it passes them, I assume it was missing the ability to be imported via require? Also, do we need to do this for react hooks as well?

"types": "./dist/index.d.ts",
"default": "./dist/index.mjs"
},
"require": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this was the main thing that was missing for node CJS imports (?)

@KyleAMathews
Copy link
Contributor Author

Also, do we need to do this for react hooks as well?

Strangely... no? Wait... I thought I'd checked it too but apparently not as yeah, it does have the same issue — https://arethetypeswrong.github.io/?p=%40electric-sql%2Freact%400.6.1

I'll update it as well in a little while.

Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit 6ad5559
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/67571255bba80a00089bc461
😎 Deploy Preview https://deploy-preview-2112--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for electric-next ready!

Name Link
🔨 Latest commit fe75071
🔍 Latest deploy log https://app.netlify.com/sites/electric-next/deploys/6757164944a07600080a4fde
😎 Deploy Preview https://deploy-preview-2112--electric-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@KyleAMathews KyleAMathews merged commit dae3b0d into main Dec 9, 2024
3 checks passed
@KyleAMathews KyleAMathews deleted the fix-build branch December 9, 2024 16:20
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.

@electric-sql/client masquerading as ESM when using node16 moduleResolution
2 participants