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

Set package sideEffects to false #287

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

federicocarboni
Copy link
Contributor

Allows bundlers to tree-shake out the temporal polyfill when it is imported but not used.

If (or when) in the future the polyfill patches the global object "sideEffects" may be changed to an array to declare the side effects of a specific entry (e.g. @js-temporal/polyfill/global has side effects while other entry points do not).

Allows bundlers to tree-shake out the temporal polyfill when imported but not used.
@12wrigja
Copy link
Contributor

There are package-local side-effects when modules are evaluated (example:

MakeIntrinsicClass(TimeZone, 'Temporal.TimeZone');
DefineIntrinsic('Temporal.TimeZone.prototype.getOffsetNanosecondsFor', TimeZone.prototype.getOffsetNanosecondsFor);
DefineIntrinsic('Temporal.TimeZone.prototype.getPossibleInstantsFor', TimeZone.prototype.getPossibleInstantsFor);
).

It is possible to convert from one Temporal type to another today using prototype methods which require references to the other module's classes being registered using these side-effects (e.g. toZonedDateTime:

return ES.CreateTemporalZonedDateTime(GetSlot(this, EPOCHNANOSECONDS), timeZone, 'iso8601');
). Would this method still work with this sideEffect flag if user code only ever imported Instant (as Instant does not and can not have a direct import on ZonedDateTime)?

@federicocarboni
Copy link
Contributor Author

sideEffects does not affect function calls. It is a hint to module bundlers that whole modules may be ignored if none of their exports are used.

Just in case, I checked built files and they are bit-by-bit identical with and without sideEffects.

@12wrigja
Copy link
Contributor

Ok - thank you for checking. Seems reasonable to me.

@12wrigja 12wrigja merged commit 44ea889 into js-temporal:main Jun 24, 2024
19 checks passed
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.

2 participants