Add esm data to package json to fix #908 #1284
Conversation
|
@matt-hernandez is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
| "engines": { | ||
| "node": ">=20.1.0" | ||
| } |
There was a problem hiding this comment.
why do we need to bump the node version here?
we try to be pretty conservative with changes since it can be hard to predict consequence on other folks CI
is it this change that means we need the changes to eslint?
There was a problem hiding this comment.
(i'm pretty sure we can support at least >=18 since that's what i'm using locally, i haven't tested with 16)
There was a problem hiding this comment.
The node version requirement is due to Cypress not running e2e tests with ESM unless it runs in node > 20.1.0. There may be another workaround, but in local, that was the only way I could get Cypress working.
ok, the version requirement bump is likely to be painful for an SDK. Node 18 isn't end of life until Apr 2025
but equally don't want to leave folk with your problem hanging...
maybe we can do this another way - we have an examples folder - you can see for e.g. the nextjs folder at https://github.com/PostHog/posthog-js/tree/main/playground/nextjs
could you open another pr with a vite/remix example - (as small as possible 😊)
that should let us see the problem and help us keep it working in future
(i've never touched remix or i'd offer to make one - but i'm guessing you'll do a better job 💖 )
|
hey @matt-hernandez 👋 thanks for this! it's a little beyond me so a few questions to get started 😊 |
| "type": "module", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/module.d.ts", | ||
| "import": "./dist/es.js", | ||
| "require": "./dist/module.js" | ||
| }, | ||
| "./react": { | ||
| "types": "./react/dist/types/index.d.ts", | ||
| "import": "./react/dist/esm/index.js", | ||
| "require": "./react/dist/umd/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
i've been sort of ignoring the module/bundling changes in JS since they seem pretty volatile and i was hoping for things to settle and i could learn it once 🤣
can you explain what this does/means?
i'm mostly wondering if we'll have unexpected consequences in other environments than you're testing for, i've found sometimes apparently benign changes throw up weird errors for folk
(my guess is this is safe but 🙈)
|
@pauldambra The node version requirement is due to Cypress not running e2e tests with ESM unless it runs in node > 20.1.0. There may be another workaround, but in local, that was the only way I could get Cypress working. The exports of the package.json files combined with Unfortunately, by converting this repo to full ESM, the changes ballooned. Pretty much anything with CommonJS format is having to change. Eslint is also completely borked after the conversion to ESM. And I think the only stable way to fix it would be to bump up ESLint 9+ and convert I’m converting this to a draft in the meantime since changing everything to ESM represents more than just a few metadata changes. |
|
@pauldambra I've created PR #1293 to demo the issue from #908 |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
I’ll close this one since we have a different one open that’s demonstrating the underlying problem |
Changes
Add necessary
package.jsondata to root and/reactfolder to fix compat issues between posthog and Remix with Vite. Fixes #908This fix is based on what I had to do at my job to make
posthog-jsandposthog-js/reactplay nicely with our Remix w/ Vite app. Locally, tests passed, however e2e tests had some flakiness. My gut instinct tells me that the intermittent failures were not due to the changes to any of thepackage.jsonfiles.Also this fix grew past what I thought it would originally be. By converting things to an ES module, several other files had to be renamed in order to be compatible with CommonJS format, specifically for ESLint. Will be watching Github actions to see if things complete without failure.
...
Checklist