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.

Sign up for GitHub

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

Jump to bottom

test(es/minifier): Add a test for ?? operator #6282

Merged
merged 16 commits into from Oct 28, 2022
Merged

test(es/minifier): Add a test for ?? operator #6282

merged 16 commits into from Oct 28, 2022

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 28, 2022 •
edited

Description:

I'm still investigating.

Related issue:

Investigation

                        &JsMinifyOptions {
                            compress: BoolOrDataConfig::from_bool(false),
                            mangle: BoolOrDataConfig::from_bool(false),
                            ..self.opts.clone()
                        }

works, while

                        &JsMinifyOptions {
                            compress: TerserCompressorOptions {
                                defaults: false,
                                ..Default::default()
                            }
                            .into(),
                            mangle: BoolOrDataConfig::from_bool(false),
                            ..self.opts.clone()
                        },

does not

I found the cause.

image

is compiled as

image

so we should preserve ??

@kdy1 kdy1 added this to the Planned milestone Oct 28, 2022
@kdy1 kdy1 self-assigned this Oct 28, 2022
@kdy1 kdy1 changed the title fix(es/minifier): Fix a bug fix(es/minifier): Don't drop ?? Oct 28, 2022
@kdy1 kdy1 marked this pull request as ready for review October 28, 2022 02:56
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
kdy1
kdy1 commented Oct 28, 2022
View reviewed changes
crates/swc_ecma_minifier/tests/fixture/next/41992/output.js Outdated
let { onTryoutClick: t , parameters: r , allowTryItOut: a , tryItOutEnabled: n , specPath: l , fn: s , getComponent: o , getConfigs: i , specSelectors: u , specActions: d , pathMethod: g , oas3Actions: v , oas3Selectors: E , operation: b } = this.props;
const S = o("parameterRow"), _ = o("TryItOutButton"), w = o("contentType"), C = o("Callbacks", !0), x = o("RequestBody", !0), A = n && a, I = u.isOAS3(), R = b.get("requestBody"), N = (0, p.default)(e = (0, ft.default)((0, p.default)(r).call(r, (e, t)=>{
const r = t.get("in");
return e[r] ?? (e[r] = []), e[r].push(t), e;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was previously e[r], e[r] = []

@kdy1 kdy1 requested a review from jridgewell October 28, 2022 02:58
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
kdy1
kdy1 commented Oct 28, 2022
View reviewed changes
Copy link
Member Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

swc-bump:

  • swc_ecma_minifier

jridgewell
jridgewell previously approved these changes Oct 28, 2022
View reviewed changes
@jridgewell
Copy link
Contributor

Is it possible to add a reduced test case?

@kdy1 kdy1 dismissed stale reviews from jridgewell and kodiakhq via 6e77c13 October 28, 2022 05:07
kdy1
kdy1 commented Oct 28, 2022
View reviewed changes
crates/swc_ecma_minifier/tests/fixture/next/41992/1/output.js Outdated
tryItOutEnabled: !1,
allowTryItOut: !0,
onChangeKey: [],
specPath: []
Copy link
Member Author

Choose a reason for hiding this comment

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

@jridgewell Do you think it's better to remove this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. If this file changes in the future, there's very little chance I'll spot the previously buggy code again.

Copy link
Member Author

@kdy1 kdy1 Oct 28, 2022 •
edited

Choose a reason for hiding this comment

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

Good point, I'll do it for other patches too

kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
@kdy1 kdy1 mentioned this pull request Oct 28, 2022
Closed
@Austaras
Copy link
Member

Has this already been fixed by #6272?

@kdy1
Copy link
Member Author

kdy1 commented Oct 28, 2022

@Austaras No, as we have removing logic in more places

@kdy1 kdy1 requested a review from jridgewell October 28, 2022 07:07
@Austaras
Copy link
Member

No, there's alreay a may_short_circuit in L668, and I've testes the input on main branch which generates identical output.

@Austaras
Copy link
Member

The ignore_return_value is really too long to parse for human...

@kdy1
Copy link
Member Author

kdy1 commented Oct 28, 2022

Oh, if then it's possible that I was using a old base branch.

kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 28, 2022
View reviewed changes
@kdy1
Copy link
Member Author

kdy1 commented Oct 28, 2022

You are right, this was fixed already and I was using an old base

@kdy1 kdy1 changed the title fix(es/minifier): Don't drop ?? test(es/minifier): Add a test for ?? operator Oct 28, 2022
@kdy1 kdy1 enabled auto-merge (squash) October 28, 2022 07:24
@kdy1 kdy1 merged commit 0376da7 into swc-project:main Oct 28, 2022
@kdy1 kdy1 deleted the next-41992 branch October 28, 2022 08:00
@kdy1 kdy1 modified the milestones: Planned, v1.3.12, v1.3.13 Nov 2, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kodiakhq kodiakhq[bot] kodiakhq approved these changes

@jridgewell jridgewell Awaiting requested review from jridgewell

Assignees

@kdy1 kdy1

Labels
None yet
Milestone
v1.3.13
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kdy1 @jridgewell @Austaras