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

feat: add support for UMD and ESM #166

Merged
merged 9 commits into from
Jan 22, 2021

Conversation

weyert
Copy link
Contributor

@weyert weyert commented Dec 23, 2020

Updated the build for posthog-js/react so it now outputs UMD and ESM

fixes #165

Changes

updated the build process for the React components so that it outputs both UMD and ESM format

Checklist

  • Tests for new code (if applicable)
  • TypeScript definitions (module.d.ts) updated and in sync with library exports (if applicable)

Updated the build for `posthog-js/react` so it now outputs UMD and ESM
@weyert
Copy link
Contributor Author

weyert commented Dec 24, 2020

@macobo @stevenphaedonos please have a look :)

react/LICENSE Outdated Show resolved Hide resolved
react/package.json Outdated Show resolved Hide resolved
react/package.json Outdated Show resolved Hide resolved
react/rollup.config.ts Outdated Show resolved Hide resolved
react/rollup.config.ts Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @weyert, I've left some comments. I was also wondering if you could provide some details about your use-case where the es2015 module was insufficient?

package.json Outdated Show resolved Hide resolved
@weyert
Copy link
Contributor Author

weyert commented Jan 11, 2021

@stevenphaedonos I have updated it

react/package.json Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Remove the redundant line in files of package.json and it looks good to me. Do you have any comments @macobo?

@macobo
Copy link
Contributor

macobo commented Jan 11, 2021

I will take a look hopefully sometime soon. On a high level I think we might end up rolling all of the builds into a single rollup file.

@weyert
Copy link
Contributor Author

weyert commented Jan 11, 2021

Remove the redundant line in files of package.json and it looks good to me. Do you have any comments @macobo?

Removed it! Thank you :)

@weyert
Copy link
Contributor Author

weyert commented Jan 13, 2021

All good?

@weyert
Copy link
Contributor Author

weyert commented Jan 19, 2021

Any news on this PR? Love to stop using my private build

@Twixes Twixes requested a review from macobo January 19, 2021 19:28
@macobo
Copy link
Contributor

macobo commented Jan 22, 2021

Sorry for this taking so long. To elaborate a bit on the cause:

  1. This not really blocking anything or adding any visible functionality in environments I could test. Afaik modern js builds only limit bundle size a bit and the package was usable in previous state.
  2. The paths of built files were not consistent with the main package and rollup.js.config was also structured differently and used a different language. There's work to be done to make all of this consistent, but this is not in the scope of this PR.
  3. There was an in-progress branch reworking how the main package is bundled which conflicted with this work.
  4. We don't have a good way to test the changes out.

I was hoping to take some time to clean all of this up properly but it seems this time is not going to be soon. I'll merge this as-is and deal with the fallout at a later date - while this does introduce a ton of technical debt and is untestable in current state, it is a useful change.

PS: We've been waiting for a response on #114 as well for months.

@macobo macobo merged commit b8ef72f into PostHog:master Jan 22, 2021
@weyert
Copy link
Contributor Author

weyert commented Jan 22, 2021

@macobo Thank you! I can confirm that #114 can be closed its all working with v1.80+ for me know as this PR indicates too :)

Sorry for not coming back to that PR .

@weyert weyert deleted the chore-improve-bundling branch January 22, 2021 17:06
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.

React component can't be easily consumed in Next.js
4 participants