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

refactor: implement signals-core with untranspiled classes #520

Closed
wants to merge 1 commit into from
Closed

refactor: implement signals-core with untranspiled classes #520

wants to merge 1 commit into from

Conversation

jviide
Copy link
Contributor

@jviide jviide commented Mar 12, 2024

This pull request converts the signals-core package to use ES6 classes instead of ES5-style prototypes. It effectively reverts the changes introduced in #160.

Notes:

  • The classes are not transpiled to ES5: The transpilation is disabled by excluding the transform-classes Babel transform when transpiling the core.
  • Some class property definitions use TypeScript's declare keyword. This avoids a redundant property assignment getting added to the class's constructor. There is a comment included that describes this reasoning.
  • Computed's prototype chain changes from Computed -> Signal to Computed -> Intermediate -> Signal. The Preact binding needed to be modified to take this into account: setting Signal.prototype.constructor to undefined gets masked and Computed.prototype.constructor is not undefined anymore. Which led to this cursed piece of code by Yours Truly.

The sizes of the build artifacts before and after this change are listed below. In general the number of bytes saved falls around 40-60 bytes, depending on the output format.

Before:
       1487 B: signals-core.js.gz
       1352 B: signals-core.js.br
       1513 B: signals-core.mjs.gz
       1378 B: signals-core.mjs.br
       1498 B: signals-core.module.js.gz
       1369 B: signals-core.module.js.br
       1561 B: signals-core.min.js.gz
       1421 B: signals-core.min.js.br

After:
       1429 B: signals-core.js.gz
       1300 B: signals-core.js.br
       1449 B: signals-core.mjs.gz
       1328 B: signals-core.mjs.br
       1438 B: signals-core.module.js.gz
       1316 B: signals-core.module.js.br
       1513 B: signals-core.min.js.gz
       1379 B: signals-core.min.js.br

Copy link

changeset-bot bot commented Mar 12, 2024

⚠️ No Changeset found

Latest commit: 00be0d4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Mar 12, 2024

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit 00be0d4
🔍 Latest deploy log https://app.netlify.com/sites/preact-signals-demo/deploys/65f0d237cc7299000833f5a3
😎 Deploy Preview https://deploy-preview-520--preact-signals-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rschristian
Copy link
Member

While the size reduction is nice, wouldn't this create a bit of a weird discrepancy? If someone's using Preact and (for some godforsaken reason) needs to support ES5, there's now this weird little bit of ES6 in their otherwise ES5 bundles.

I'm just not sure this makes much sense to land on its own.

@marvinhagemeister
Copy link
Member

Ah totally forgot about ES5. Yeah that might make it difficult to merge this PR as long as Preact itself ships like that.

@marvinhagemeister marvinhagemeister self-requested a review March 15, 2024 08:05
@marvinhagemeister marvinhagemeister dismissed their stale review March 15, 2024 08:06

forgot about es5

@jviide
Copy link
Contributor Author

jviide commented Mar 15, 2024

Yep, good points.

@jviide jviide closed this Mar 15, 2024
@jviide jviide deleted the patch-02 branch March 15, 2024 13:35
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.

3 participants