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: support tree shaking #829

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

feat: support tree shaking #829

wants to merge 36 commits into from

Conversation

gclark-eightfold
Copy link
Contributor

@gclark-eightfold gclark-eightfold commented May 10, 2024

SUMMARY:

Adds tree-shaking support for apps using Octuple. Core changes:

  1. Add explicit sideEffects to package.json for CJS tree shaking.
  2. Switch from Webpack to Rollup to support exporting ESM.

For the Webpack -> Rollup change, it may be possible to export ESM from webpack, but most sources I found said exporting ESM from webpack was not supported. Additionally, after examining quite a number of specifically react component libraries, it seemed like Rollup was the most common tool for this job.

Testing on Eightfold for the careerHub entry point only yielded a -20.9% decrease in the bundle size for the octuple chunk compared to v2.51.1

Here is the bundle analysis diff in Blindfold.ai after following the steps outlined in the Test Plan

Before

Before

After

After

GITHUB ISSUE (Open Source Contributors)

NA

JIRA TASK (Eightfold Employees Only):

ENG-55139

CHANGE TYPE:

  • Bugfix Pull Request
  • Feature Pull Request

TEST COVERAGE:

  • Tests for this change already exist
  • I have added unittests for this change

TEST PLAN:

This test plan relies on running Blindfold locally, but can be done in isolation if desired

  1. Clone the https://github.com/EightfoldAI/blindfold repo locally
  2. In the root of the repo, add the following .env file. (If you have one already, just add ANALYZE_BUNDLES=1
ANALYZE_BUNDLES=1
BACKEND_BASE_URL="https://app.eightfold.ai"
NEXT_PUBLIC_HOST_URL="https://localhost:8000"
NEXTAUTH_URL="https://localhost:8000"
  1. Run git checkout gclark/octuple-bundle-test to pull these changes.
  2. Install deps with pnpm install. (Run npm install -g pnpm if you do not have pnpm installed)
  3. Run pnpm build --filter nextjs to build the Blindfold app.
  4. Once complete open the file located in apps/nextjs/.next/analyze/client.html in your web browser.
    • The bundle analysis does not show the entire octuple.js bundle present.
  5. Run pnpm start --filter nextjs and go to http://localhost:8000/octuple
    • Verify the Octuple primary button is present on the page and styled correctly.
  6. Remove the patch with pnpm patch-remove @eightfold.ai/octuple
  7. Run pnpm build --filter nextjs again.
  8. Once complete open the file located in apps/nextjs/.next/analyze/client.html in your web browser.
    • The bundle analysis does show the entire octuple.js bundle present.

.github/workflows/build.yml Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
rollup.config.mjs Outdated Show resolved Hide resolved
rollup.config.mjs Outdated Show resolved Hide resolved
@dkilgore-eightfold
Copy link
Collaborator

Need to bump node in .codesanbox dir

@dkilgore-eightfold
Copy link
Collaborator

@gclark-eightfold You may be correct:

webpack/webpack#2933

Copy link

codesandbox-ci bot commented May 10, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.70%. Comparing base (45e0c60) to head (3730bf5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #829   +/-   ##
=======================================
  Coverage   84.69%   84.70%           
=======================================
  Files        1098     1098           
  Lines       20417    20417           
  Branches     7718     7718           
=======================================
+ Hits        17293    17294    +1     
+ Misses       3040     3039    -1     
  Partials       84       84           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@dkilgore-eightfold
Copy link
Collaborator

Also removed this line and its import to fix the circular dep issue accross bundles:

https://github.com/EightfoldAI/octuple/blob/main/src/shared/utilities/flexGapSupported.ts#L7

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

2 participants