Skip to content

Conversation

@sugarmanz
Copy link
Member

@sugarmanz sugarmanz commented Aug 7, 2024

Currently, the plugin native bundles contain an entire copy of Player - this PR externalizes the Player from the native bundles, reducing size burden of the native bundles.

Change Type (required)

Indicate the type of change your pull request is:

  • patch
  • minor
  • major

Does your PR have any documentation updates?

  • Updated docs
  • No Update needed
  • Unable to update docs

@sugarmanz sugarmanz added the patch Increment the patch version when merged label Aug 7, 2024
@sugarmanz
Copy link
Member Author

/canary

@KetanReddy
Copy link
Member

Can we do this as a part of the rule to minimize the config needed on the usage side?

@sugarmanz
Copy link
Member Author

Can we do this as a part of the rule to minimize the config needed on the usage side?

It would likely require pulling more than what I touch out of this repo, since we define the template in this repo too:
https://github.com/player-ui/player/pull/483/files#diff-d20cc7f8a4827655e2b6e1c235a7ed8e4f4fa9446fb0c84e3f71da57cfef2f0eR19

@KetanReddy
Copy link
Member

Can we do this as a part of the rule to minimize the config needed on the usage side?

It would likely require pulling more than what I touch out of this repo, since we define the template in this repo too: https://github.com/player-ui/player/pull/483/files#diff-d20cc7f8a4827655e2b6e1c235a7ed8e4f4fa9446fb0c84e3f71da57cfef2f0eR19

Ah, sorry. I thought we had some base tsup config that was expanded in our rules.

load("@npm//:defs.bzl", "npm_link_all_packages")
load("//tools:defs.bzl", "NATIVE_BUILD_DEPS", "tsup_config", "vitest_config")

# TODO: Would be nice to macro all the native things to avoid missing this setup (consequence is bundling Player in a plugin bundle)
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any estimate for how much effort this would require? If its not huge would it be worth it to just do it now?

if (process.env.PLAYER_NATIVE_BUNDLE) {
overrides.esbuildPlugins = [ESBuildPluginExternalGlobal.externalGlobalPlugin({
// TODO: Would be cool to automatically populate this given all the native bundles we create
"@player-ui/player": "globalThis.Player",
Copy link
Member Author

Choose a reason for hiding this comment

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

@KetanReddy KetanReddy marked this pull request as draft July 25, 2025 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants