refactor: change public const enums to enums#51670
Closed
frigus02 wants to merge 1 commit intoangular:mainfrom
Closed
refactor: change public const enums to enums#51670frigus02 wants to merge 1 commit intoangular:mainfrom
frigus02 wants to merge 1 commit intoangular:mainfrom
Conversation
Contributor
|
@frigus02 Looks like you'll need to fix your commit messages to match lint requirements. |
8aa8711 to
3141fc3
Compare
Contributor
Author
|
The lint is very picky 🙂. I squashed the commits. Hope that's okay. Is this change okay in general? Depending on how you see it, this could be a breaking change. But it would be really really really good to have. |
JoostK
reviewed
Sep 6, 2023
packages/common/http/src/response.ts
Outdated
Member
There was a problem hiding this comment.
This one in particular is super sad not to have as const enum, since it has so many variants and therefore quite a code size overhead.
Contributor
Author
There was a problem hiding this comment.
Agree. Google internal we use Closure Compiler, which inlines enums anyway, so this is not an issue. Do we know what the more popular bundlers & minifiers do? Can they inline enums, too?
Contributor
Author
There was a problem hiding this comment.
Looks like esbuild does inline them:
$ cat test.ts
const enum Const { A = 1 }
enum Regular { A = 1 }
console.log(Const.A, Regular.A);
export {};
$ ./node_modules/.bin/esbuild ./test.ts --bundle --minify
(()=>{console.log(1,1);})();
Member
There was a problem hiding this comment.
Oh, that is cool! It even works with code splitting enabled in esbuild. It's been supported since 0.14.9.
3 tasks
3 tasks
alxhub
approved these changes
Oct 18, 2023
Member
alxhub
left a comment
There was a problem hiding this comment.
Reviewed-for: global-approvers
Okaying this as a public API change post-freeze for v17. It should have no impact on consumers (besides bundle size, which depends on the inlining behavior and as mentioned in comments, seems to be optimal with esbuild & Closure, at least).
Long-term, relying on const enum semantics seems unnecessary. It should be the optimizer's job to inline constants and remove unused references, not TypeScript's.
977c2af to
f541a99
Compare
frigus02
commented
Oct 19, 2023
Contributor
Author
There was a problem hiding this comment.
Diff caused by enum
--- /tmp/old.js 2023-10-19 08:08:33.412077045 +0000
+++ /tmp/new.js 2023-10-19 08:09:01.282909097 +0000
@@ -1,56 +1,66 @@
'use strict';
(self.webpackChunkanimations = self.webpackChunkanimations || []).push([
['default-node_modules_angular_animations_fesm2022_animations_mjs'], {
- 825: (E, a, h) => {
- h.d(a, {
- SB: () => y,
- X$: () => u,
- ZE: () => g,
+ 825: (w, h, l) => {
+ l.d(h, {
+ SB: () => m,
+ X$: () => _,
+ ZE: () => S,
ZN: () => D,
- eR: () => f,
- jt: () => d,
- k1: () => A,
- l3: () => l,
- oB: () => c,
- vP: () => _
+ eR: () => y,
+ jt: () => c,
+ k1: () => E,
+ l3: () => u,
+ mo: () => i,
+ oB: () => f,
+ vP: () => d
});
- const l = '*';
- function u(n, t) {
+ var i = function(s) {
+ return s[s.State = 0] = 'State', s[s.Transition = 1] = 'Transition',
+ s[s.Sequence = 2] = 'Sequence', s[s.Group = 3] = 'Group',
+ s[s.Animate = 4] = 'Animate', s[s.Keyframes = 5] = 'Keyframes',
+ s[s.Style = 6] = 'Style', s[s.Trigger = 7] = 'Trigger',
+ s[s.Reference = 8] = 'Reference', s[s.AnimateChild = 9] = 'AnimateChild',
+ s[s.AnimateRef = 10] = 'AnimateRef', s[s.Query = 11] = 'Query',
+ s[s.Stagger = 12] = 'Stagger', s
+ }(i || {});
+ const u = '*';
+ function _(s, t) {
return {
- type: 7, name: n, definitions: t, options: {}
+ type: i.Trigger, name: s, definitions: t, options: {}
}
}
- function d(n, t = null) {
+ function c(s, t = null) {
return {
- type: 4, styles: t, timings: n
+ type: i.Animate, styles: t, timings: s
}
}
- function _(n, t = null) {
+ function d(s, t = null) {
return {
- type: 2, steps: n, options: t
+ type: i.Sequence, steps: s, options: t
}
}
- function c(n) {
+ function f(s) {
return {
- type: 6, styles: n, offset: null
+ type: i.Style, styles: s, offset: null
}
}
- function y(n, t, i) {
+ function m(s, t, e) {
return {
- type: 0, name: n, styles: t, options: i
+ type: i.State, name: s, styles: t, options: e
}
}
- function f(n, t, i = null) {
+ function y(s, t, e = null) {
return {
- type: 1, expr: n, animation: t, options: i
+ type: i.Transition, expr: s, animation: t, options: e
}
}
class D {
- constructor(t = 0, i = 0) {
+ constructor(t = 0, e = 0) {
this._onDoneFns = [], this._onStartFns = [], this._onDestroyFns = [],
this._originalOnDoneFns = [], this._originalOnStartFns = [], this._started = !1,
this._destroyed = !1, this._finished = !1, this._position = 0, this.parentPlayer = null,
- this.totalTime = t + i
+ this.totalTime = t + e
}
_onFinish() {
this._finished ||
@@ -99,23 +109,23 @@
return this.totalTime ? this._position / this.totalTime : 1
}
triggerCallback(t) {
- const i = 'start' == t ? this._onStartFns : this._onDoneFns;
- i.forEach(e => e()), i.length = 0
+ const e = 'start' == t ? this._onStartFns : this._onDoneFns;
+ e.forEach(n => n()), e.length = 0
}
}
- class g {
+ class S {
constructor(t) {
this._onDoneFns = [], this._onStartFns = [], this._finished = !1, this._started = !1,
this._destroyed = !1, this._onDestroyFns = [], this.parentPlayer = null,
this.totalTime = 0, this.players = t;
- let i = 0, e = 0, s = 0;
+ let e = 0, n = 0, r = 0;
const o = this.players.length;
- 0 == o ? queueMicrotask(() => this._onFinish()) : this.players.forEach(r => {
- r.onDone(() => {++i == o && this._onFinish()}),
- r.onDestroy(() => {++e == o && this._onDestroy()}),
- r.onStart(() => {++s == o && this._onStart()})
+ 0 == o ? queueMicrotask(() => this._onFinish()) : this.players.forEach(a => {
+ a.onDone(() => {++e == o && this._onFinish()}),
+ a.onDestroy(() => {++n == o && this._onDestroy()}),
+ a.onStart(() => {++r == o && this._onStart()})
}),
- this.totalTime = this.players.reduce((r, S) => Math.max(r, S.totalTime), 0)
+ this.totalTime = this.players.reduce((a, P) => Math.max(a, P.totalTime), 0)
}
_onFinish() {
this._finished ||
@@ -165,26 +175,26 @@
this._started = !1
}
setPosition(t) {
- const i = t * this.totalTime;
- this.players.forEach(e => {
- const s = e.totalTime ? Math.min(1, i / e.totalTime) : 1;
- e.setPosition(s)
+ const e = t * this.totalTime;
+ this.players.forEach(n => {
+ const r = n.totalTime ? Math.min(1, e / n.totalTime) : 1;
+ n.setPosition(r)
})
}
getPosition() {
const t =
- this.players.reduce((i, e) => null === i || e.totalTime > i.totalTime ? e : i, null);
+ this.players.reduce((e, n) => null === e || n.totalTime > e.totalTime ? n : e, null);
return null != t ? t.getPosition() : 0
}
beforeDestroy() {
this.players.forEach(t => {t.beforeDestroy && t.beforeDestroy()})
}
triggerCallback(t) {
- const i = 'start' == t ? this._onStartFns : this._onDoneFns;
- i.forEach(e => e()), i.length = 0
+ const e = 'start' == t ? this._onStartFns : this._onDoneFns;
+ e.forEach(n => n()), e.length = 0
}
}
- const A = '!'
+ const E = '!'
}
}
]);
\ No newline at end of file
Contributor
Author
|
Thanks for the review, Alex. I rebased and fixed the tests. |
Angular recently gained a local compilation mode (see commit 345dd6d). This is intended to be used with the TypeScript compiler option isolatedModules, which bans imports of const enums. This changes all const enums tagged with @publicapi to regular enums. Fixes angular#46240
Contributor
|
This PR was merged into the repository by commit ebbc7a2. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
ChellappanRajan
pushed a commit
to ChellappanRajan/angular
that referenced
this pull request
Jan 23, 2024
Angular recently gained a local compilation mode (see commit 345dd6d). This is intended to be used with the TypeScript compiler option isolatedModules, which bans imports of const enums. This changes all const enums tagged with @publicapi to regular enums. Fixes angular#46240 PR Close angular#51670
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Angular recently gained a local compilation mode (see commit 345dd6d). This is intended to be used with the TypeScript compiler option isolatedModules, which bans imports of const enums.
This changes all const enums tagged with @publicapi to regular enums.
Fixes #46240
PR Checklist
PR Type
What is the current behavior?
Issue Number: #46240
Importing any of those const enums under
isolatedModulesresults in the errorWhat is the new behavior?
No error.
Does this PR introduce a breaking change?