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
Improve output of using
#15959
Improve output of using
#15959
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56136/ |
0e10e0a
to
376555f
Compare
return { | ||
s: [], // stack | ||
e: empty, // error | ||
using: function (value, isAwait) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this PR is worth doing, but anyway we can improve this helper:
- use
u
andd
rather thanusing
anddispose
s
can be a local variable, rather than a property- you might split
u
inu
anda
("using" and "await using"), to save on the second parameter
CI errors are funny. I will investigate. |
We do not support having two helpers with two top-level variables having the same name 😁 |
@@ -0,0 +1,78 @@ | |||
/* @minVersion 7.23.0 */ | |||
|
|||
export default function _usingCtxFactory() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you wrap the old helper rather than the new one, so that the new one remains smaller?
} | ||
|
||
function err(e) { | ||
error = error != empty ? new dispose_SuppressedError(e, error) : e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error = error != empty ? new dispose_SuppressedError(e, error) : e; | |
error = error !== empty ? new dispose_SuppressedError(e, error) : e; |
If something does throw "[object Object]"
, error == empty
will be true :P
while ((r = stack.pop())) { | ||
try { | ||
var r, | ||
p = r.d.call(r.v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The var r
here is incredibly confusing, I was initially surprised that it's not undefined
:P Unfortunately Terser cannot do this optimization for us so it's fine to do it manually, but I opened terser/terser#1446.
Also, let's rename r
to resource
and p
to disposalResult
since we now mangle variables anyway 😉
1f5f6f2
to
bb83ab5
Compare
bb83ab5
to
8ba69ba
Compare
t.addComment( | ||
replacement.block.body[0], | ||
"leading", | ||
"u: using(obj, isAwait), d: dispose()", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally do not add comments explaining the transpiled output. Though we could do so under a verbose: boolean
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove them and open a separate PR if needed in the future.
8ba69ba
to
d72e27c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a todo item in the make new-version-checklist
?
Since we have to update the minVersion
data of the usingCtx
helper.
f8a0483
to
0defb46
Compare
var empty = {}, | ||
stack = []; | ||
function using(isAwait, value) { | ||
if (value != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used v!=null
here because it doesn't look like people would use document.all
here.
And https://github.com/babel/babel/pull/15959/files#diff-cd9817b5f60ed5ac93a50bd1667f5f8216d36a6d1eb52ade7ad3ebe6bcaaf21cR34
0defb46
to
b512c42
Compare
var _disposeSuppressedError = | ||
typeof SuppressedError === "function" | ||
? // eslint-disable-next-line no-undef | ||
SuppressedError | ||
: (function (suppressed: boolean, error: Error) { | ||
var err = new Error() as SuppressedError; | ||
err.name = "SuppressedError"; | ||
err.suppressed = suppressed; | ||
err.error = error; | ||
return err; | ||
} as SuppressedErrorConstructor), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed from ts's implementation because theirs is much simpler, I think the behavior has some differences in the prototype, but I'm not sure who is right and if it is acceptable.
Also sorry for making these changes after the review. :) @JLHwung
ts helper
var __disposeResources = (this && this.__disposeResources) || (function (SuppressedError) {
return function (env) {
function fail(e) {
env.error = env.hasError ? new SuppressedError(e, env.error, "An error was suppressed during disposal.") : e;
env.hasError = true;
}
function next() {
while (env.stack.length) {
var rec = env.stack.pop();
try {
var result = rec.dispose && rec.dispose.call(rec.value);
if (rec.async)
return Promise.resolve(result).then(next, function (e) {
fail(e);
return next();
});
} catch (e) {
fail(e);
}
}
if (env.hasError)
throw env.error;
}
return next();
};
})(typeof SuppressedError === "function" ? SuppressedError : function (error, suppressed, message) {
var e = new Error(message);
return e.name = "SuppressedError",
e.error = error,
e.suppressed = suppressed,
e;
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By looking at it, I think the previous implementation was more spec-compliant, but it seems like ts's behavior is sufficient for use here.
Since we did not assign a value to SuppressedError
, users should not use instanceof
to compare it.
typeof SuppressedError === "function" | ||
? // eslint-disable-next-line no-undef | ||
SuppressedError | ||
: (function (suppressed: boolean, error: Error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I realized that our two parameters were reversed, and it is not boolean. : :)
a291a24
to
d0ec5ed
Compare
d0ec5ed
to
b1187c7
Compare
f98d8d3
to
334da77
Compare
Since rebasing made this PR more complicated, I removed the |
"using declarations can only be used with objects, functions, null, or undefined.", | ||
); | ||
} | ||
// core-js-pure uses Symbol.for for polyfilling well-known symbols |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, but maybe users are importing "things that can be disposed" from libraries that have been compiled with core-js, and that thus use Symbol.for("Symbol.dispose")
as their protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! As a follow-up PR, please remove the old transform for Babel 8.
334da77
to
5d8432f
Compare
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@babel/traverse](https://babel.dev/docs/en/next/babel-traverse) ([source](https://github.com/babel/babel)) | resolutions | patch | [`7.23.7` -> `7.23.9`](https://renovatebot.com/diffs/npm/@babel%2ftraverse/7.23.7/7.23.9) | --- ### Release Notes <details> <summary>babel/babel (@​babel/traverse)</summary> ### [`v7.23.9`](https://github.com/babel/babel/blob/HEAD/CHANGELOG.md#v7239-2024-01-25) [Compare Source](babel/babel@v7.23.7...v7.23.9) ##### 🐛 Bug Fix - `babel-helper-transform-fixture-test-runner`, `babel-plugin-transform-function-name`, `babel-plugin-transform-modules-systemjs`, `babel-preset-env` - [#​16225](babel/babel#16225) fix: `systemjs` re-traverses helpers ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helper-create-class-features-plugin`, `babel-plugin-proposal-decorators` - [#​16226](babel/babel#16226) Improve decorated private method check ([@​JLHwung](https://github.com/JLHwung)) - `babel-plugin-proposal-decorators`, `babel-plugin-transform-async-generator-functions`, `babel-plugin-transform-runtime`, `babel-preset-env` - [#​16224](babel/babel#16224) Properly sort `core-js@3` imports ([@​nicolo-ribaudo](https://github.com/nicolo-ribaudo)) - `babel-traverse` - [#​15383](babel/babel#15383) fix: Don't throw in `getTypeAnnotation` when using TS+inference ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) - Other - [#​16210](babel/babel#16210) \[eslint] Fix `no-use-before-define` for class ref in fields ([@​nicolo-ribaudo](https://github.com/nicolo-ribaudo)) ##### 🏠 Internal - `babel-core`, `babel-parser`, `babel-template` - [#​16222](babel/babel#16222) Migrate `eslint-parser` to cts ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-types` - [#​16213](babel/babel#16213) Remove `@babel/types` props that are not produced by the parser ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) ##### 🏃♀️ Performance - `babel-parser` - [#​16072](babel/babel#16072) perf: Improve parser performance for typescript ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) ##### 🔬 Output optimization - `babel-helper-create-class-features-plugin`, `babel-plugin-proposal-decorators`, `babel-plugin-proposal-destructuring-private`, `babel-plugin-proposal-pipeline-operator`, `babel-plugin-transform-class-properties`, `babel-plugin-transform-class-static-block`, `babel-plugin-transform-new-target`, `babel-plugin-transform-parameters`, `babel-plugin-transform-private-methods`, `babel-preset-env` - [#​16218](babel/babel#16218) Improve temporary variables for decorators ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) - `babel-helpers`, `babel-plugin-proposal-explicit-resource-management`, `babel-runtime-corejs2`, `babel-runtime-corejs3`, `babel-runtime` - [#​15959](babel/babel#15959) Improve output of `using` ([@​liuxingbaoyu](https://github.com/liuxingbaoyu)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=--> Reviewed-on: https://gitea.vylpes.xyz/RabbitLabs/random-bunny/pulls/142 Reviewed-by: Vylpes <ethan@vylpes.com> Co-authored-by: Renovate Bot <renovate@vylpes.com> Co-committed-by: Renovate Bot <renovate@vylpes.com>
Fixes #1, Fixes #2
To be honest I think this may not be an improvement over bundled + minimized size, but it should make the output easier to view.