Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

Don't use importHelpers in react-effect #1651

Closed
wants to merge 2 commits into from

Conversation

9at8
Copy link
Contributor

@9at8 9at8 commented Oct 9, 2020

Description

Using import helpers requires us to depend on the tslib package. Since react-effect is transpiled to cjs, it's not tree shakable. The tslib package is about 2 kb, which is roughly twice the size of the actual library. This PR removes that dependency to half the package size.

Before After
~ 3.6 kb ~ 1.8 kb
Screen Shot 2020-10-08 at 20 44 54 Screen Shot 2020-10-08 at 20 45 10

The helpers that were imported from the tslib package:

What Where
__importDefault ./src/context
__awaiter ./src/manager
__generator ./src/manager
__read ./src/manager
__spread ./src/manager
__awaiter ./src/server
__generator ./src/server
__importDefault ./src/server

Type of change

  • @shopify/react-effect Patch: Bug (non-breaking change which fixes an issue)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

Using import helpers requires us to depend on the tslib package. Since react-effect is transpiled to cjs, it's not tree shakable. The tslib package is about 2 kb, which is roughly twice the size of the actual library. This PR removes that dependency so reduce the package size.
@9at8
Copy link
Contributor Author

9at8 commented Oct 9, 2020

What about the previous changelog entries? For 3.2.2 -> 3.2.12?

@atesgoral atesgoral requested a review from a team October 13, 2020 12:37
@@ -23,9 +23,6 @@
"url": "https://github.com/Shopify/quilt/issues"
},
"homepage": "https://github.com/Shopify/quilt/blob/master/packages/react-effect/README.md",
"dependencies": {
"tslib": "^1.9.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where were we using this before? Was it just accidentally there?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused. The tslib docs mention

this means delivering users smaller files on average, as well as less runtime overhead.

Shouldn't having this be more beneficial? 🤔

@BPScott
Copy link
Member

BPScott commented Oct 26, 2020

In #1657 @alexandcote bumped tslib to be > 1.14 - which added esmodule support.

Update your app to the latest version of tslib and this should be treeshaken away.


Even without this new esmodules support I don't think this PR is the right approach - sure this size difference is not an ideal look for react-effect on its own but the wins of using tslib comes when several modules all use it together. With this PR as soon as you include any other package from this library (and shopify apps depends upon several) you depend upon the helpers in tslib AND the helpers inlined into the react-effect package. And because of this duplication you end up with a net bundle size increase.

@9at8
Copy link
Contributor Author

9at8 commented Oct 26, 2020

@BPScott This makes sense. I planned on closing this earlier. Sorry for the delay!

@9at8 9at8 closed this Oct 26, 2020
@9at8 9at8 deleted the adi-react-effect-remove-tslib branch October 26, 2020 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants