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

Tree-Shaking in React Output Target #359

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

drakegens
Copy link

@drakegens drakegens commented Jun 16, 2023

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally for affected output targets
  • Tests (npm test) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Tree-Shaking is not working for react-output-target

Issue URL: #255

Web Components exports will be split into separate files, enabling tree-shaking

Does this introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Author

Choose a reason for hiding this comment

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

I should revert this file.

@@ -19,7 +20,7 @@ export async function reactProxyOutput(
const filteredComponents = getFilteredComponents(outputTarget.excludeComponents, components);
const rootDir = config.rootDir as string;
const pkgData = await readPackageJson(rootDir);

//here
Copy link
Author

Choose a reason for hiding this comment

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

rm

@drakegens
Copy link
Author

working on fixing/adding tests now.

@drakegens
Copy link
Author

tests added.

@drakegens
Copy link
Author

Would love some feedback from @JessicaSachs or @tanner-reits or @thetaPC :)

@tanner-reits tanner-reits removed the request for review from JessicaSachs July 5, 2023 17:20
Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

Hey @drakegens 👋

Sorry for the delay here! I've been poking at this as I've had time over the last few weeks. Haven't dived into the code changes too much, but am having some issues confirming the initial treeshaking issue. Testing some builds of the Ionic Framework with this implementation and the existing React output target isn't producing a difference in bundle sizes when analyzing with a tool like the Rollup bundle analyzer. Granted, the Ionic Framework does have some additional build/bundle steps that are influencing the final distributed code.

I tried to test using the Crayons repo in the Github issue, but there are quite a few issues with getting started in that project (probably me doing something wrong). If you have a better reproduction case (or we can reach out in the Github issue), I'm happy to check things out there.

Aside from that, I was seeing some errors when testing in the Ionic Framework after generating the React proxies and then trying to build the React package. It appears this was just an issue with import statements getting generated when export statements should have been created in the specified proxiesFile for the output target config. But, that's a small issue we can work through if/when we have a good reproduction.

Please let me know if you have questions or want clarification on anything! Hopefully we can get this squared away quickly!

@drakegens
Copy link
Author

drakegens commented Jul 20, 2023

Hey there @tanner-reits!

Sorry for the delay.

I was able to see a ~1mb decrease in our bundle size (an app that consumes web components built with Stencil and wrapped via React Output Target). Using webpack-bundle-analyzer. Unfortunately I can't share my employer's code. So we'll need to find another reproduction case.

I could try and use the Crayons project too.

@ReneWerner87
Copy link

@drakegens @tanner-reits any progress on this ?

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