posthog-js icon indicating copy to clipboard operation
posthog-js copied to clipboard

Add esm data to package json to fix #908

Open matt-hernandez opened this issue 1 year ago • 3 comments

Changes

Add necessary package.json data to root and /react folder to fix compat issues between posthog and Remix with Vite. Fixes #908

This fix is based on what I had to do at my job to make posthog-js and posthog-js/react play 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 the package.json files.

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

  • [ *] Tests for new code (see advice on the tests we use)
  • [ *] Accounted for the impact of any changes across different browsers
  • [ *] Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

matt-hernandez avatar Jul 03 '24 21:07 matt-hernandez

@matt-hernandez is attempting to deploy a commit to the PostHog Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Jul 03 '24 21:07 vercel[bot]

hey @matt-hernandez 👋

thanks for this! it's a little beyond me so a few questions to get started 😊

pauldambra avatar Jul 04 '24 09:07 pauldambra

@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 ”type”: “module” help to label this package as an ESM compatible package, while also pointing bundlers that rely on CommonJS/UMD format which files to go to for their respective format type.

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 .eslintrc.js to the more recommended eslint.config.js file with flat config.

I’m converting this to a draft in the meantime since changing everything to ESM represents more than just a few metadata changes.

matt-hernandez avatar Jul 04 '24 18:07 matt-hernandez

@pauldambra I've created PR #1293 to demo the issue from #908

matt-hernandez avatar Jul 08 '24 18:07 matt-hernandez

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 stale label – otherwise this will be closed in another week.

posthog-bot avatar Jul 16 '24 09:07 posthog-bot

I’ll close this one since we have a different one open that’s demonstrating the underlying problem

matt-hernandez avatar Jul 16 '24 12:07 matt-hernandez