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

Create Fluent-northstar wrapper packlet #2983

Merged
merged 11 commits into from
Apr 26, 2023
Merged

Conversation

emlynmac
Copy link
Member

What

Add a new internal wrapper packlet, which will be the home of all northstar dependencies during the interim migration to support React 18

Why

#1900 (comment)

How Tested

Build and package

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

Chat bundle size is not changed.

  • Current size: 9839649
  • Base size: 9839649
  • Diff size: 0

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

Calling bundle size is not changed.

  • Current size: 9797457
  • Base size: 9797457
  • Diff size: 0

@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

CallWithChat bundle size is not changed.

  • Current size: 10191480
  • Base size: 10191480
  • Diff size: 0

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Copy link
Member

@PorterNan PorterNan left a comment

Choose a reason for hiding this comment

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

This is a creative way to solve the fluent northstar problem!

There are 3 questions in my mind needs answers for this:

  1. The final chat/calling bundle(contoso app bundle, not our sdk package) would be surely bigger, how much bigger it will be?
  2. Is it possible to avoid the bundle size increase for calling app? Cause it is not fluent northstar related
  3. In contoso app (samples/chat as example), I guess they will have to download 2 dependencies of react? How do they choose which react version they wanna use? Is there global conflicts for these 2 versions?

I guess we can get some answers when we hook this up with our existing packages and send another PR to build it. That PR will build the calling app and show how much bigger it is, like this:
image

@JamesBurnside
Copy link
Member

JamesBurnside commented Apr 25, 2023

  1. The final chat/calling bundle(contoso app bundle, not our sdk package) would be surely bigger, how much bigger it will be?

Depends on how well we can support treeshaking in the bundle, ideally this package will create a dist-esm folder that can be treeshaken just like any package.json.

  1. Is it possible to avoid the bundle size increase for calling app? Cause it is not fluent northstar related

Again depends on treeshaking support. If we have to go down the route of one bundled js file then we can't avoid it - technically it should be possible but we'll see how difficult it will be. Hopefully the increase will be small though as only the chat and it's dependencies would be added to a calling bundle (as calling already has to bundle the n* theme provider and uses components like Ref from n*)

  1. In contoso app (samples/chat as example), I guess they will have to download 2 dependencies of react? How do they choose which react version they wanna use? Is there global conflicts for these 2 versions?

Nope! Only one version of react will be used. Essentially what this packlet will hopefully do is re-bundle the necessary exports without the peer dependency restriction. It'll still require a peer dependency on react but we'll specificy this peer dep can be react 18. The n* components will now use react 18 and we need to bug bash any possible regressions with doing so (very quick POC we're not seeing any issues so hopefully we won't run into big blockers)

To add to this, this PR is just adding the packlet, the bundling implementation will come in future PR.

@@ -10,7 +10,8 @@
"@internal/calling-stateful-client": ["../../calling-stateful-client/src"],
"@internal/chat-stateful-client": ["../../chat-stateful-client/src"],
"@internal/react-components": ["../../react-components/src"],
"@internal/react-composites": ["../../react-composites/src"]
"@internal/react-composites": ["../../react-composites/src"],
"@internal/northstar-wrapper": ["../../northstar-wrapper/src"]
Copy link
Member

Choose a reason for hiding this comment

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

heads up we might have to add this at a future time to the storybook and sample app tsconfigs&webpack configs -- but lets only do that when/if we need to

@github-actions
Copy link
Contributor

@PorterNan
Copy link
Member

PorterNan commented Apr 26, 2023

  1. The final chat/calling bundle(contoso app bundle, not our sdk package) would be surely bigger, how much bigger it will be?

Depends on how well we can support treeshaking in the bundle, ideally this package will create a dist-esm folder that can be treeshaken just like any package.json.

  1. Is it possible to avoid the bundle size increase for calling app? Cause it is not fluent northstar related

Again depends on treeshaking support. If we have to go down the route of one bundled js file then we can't avoid it - technically it should be possible but we'll see how difficult it will be. Hopefully the increase will be small though as only the chat and it's dependencies would be added to a calling bundle (as calling already has to bundle the n* theme provider and uses components like Ref from n*)

  1. In contoso app (samples/chat as example), I guess they will have to download 2 dependencies of react? How do they choose which react version they wanna use? Is there global conflicts for these 2 versions?

Nope! Only one version of react will be used. Essentially what this packlet will hopefully do is re-bundle the necessary exports without the peer dependency restriction. It'll still require a peer dependency on react but we'll specificy this peer dep can be react 18. The n* components will now use react 18 and we need to bug bash any possible regressions with doing so (very quick POC we're not seeing any issues so hopefully we won't run into big blockers)

To add to this, this PR is just adding the packlet, the bundling implementation will come in future PR.

Thanks for adding details of this wrapper - so I kinda misunderstood the purpose of this wrapper, lemme try to update my understanding:

  1. Currently we are doing communication-react -> n* -> react 16, and because n* is the direct dependencies of us, so we don't have a way to avoid react 16 (cause it is directly defined in its package.json)
  2. In this wrapper (not this PR), what we try to do is communication-react -> n* wrapper (bundling the n* code) -> no need for react 16, so it's kinda like we import all the code from n* and do some kinda webpack, and rebundle it into a new package?

So I guess we are going to add webpack or some other bundling into this(for now this PR is only opening the new package)? If that is what we are doing then we might need to set React and some other packages as external dependencies, to avoid duplicate dependencies, like this: https://stackoverflow.com/questions/33154285/exclude-react-from-webpack-bundle

Copy link
Member

@PorterNan PorterNan left a comment

Choose a reason for hiding this comment

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

Curious on what the dist-esm will look like after we finish the bundling code implementation, please add me to reviewer list when it is ready

@PorterNan
Copy link
Member

Curious on what the dist-esm will look like after we finish the bundling code implementation, please add me to reviewer list when it is ready

Would be nice to see a webpack analyzer dependency map or something similar in the end, if we solve the duplicated bundling problem, this would be a perfect workaround

@emlynmac emlynmac merged commit c5c0a2b into main Apr 26, 2023
29 checks passed
@emlynmac emlynmac deleted the emlyn/react18-northstar-wrapper branch April 26, 2023 14:56
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.

None yet

3 participants