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

[Bug]: Nullish coalescing operator logic in useWindowEx is incorrect #31929

Open
2 tasks done
Jamie0 opened this issue Jul 5, 2024 · 3 comments
Open
2 tasks done

[Bug]: Nullish coalescing operator logic in useWindowEx is incorrect #31929

Jamie0 opened this issue Jul 5, 2024 · 3 comments
Assignees
Labels
Fluent UI react (v8) Issues about @fluentui/react (v8)

Comments

@Jamie0
Copy link

Jamie0 commented Jul 5, 2024

Library

React / v8 (@fluentui/react)

System Info

N/A

Are you reporting Accessibility issue?

None

Reproduction

See lib/utilities/dom.js on NPM

Bug Description

The published @fluentui/react package is incorrectly transpiling ES syntax in the module/lib builds. Specifically, this seems to affect use of the new nullish coalescing operator, which the transpiler is giving a lower order of operation than the ternary operator.

I discovered this after troubleshooting why the useWindow context didn't appear to be working, when rendering @fluentui/react components inside a popup window.

Actual Behavior

useWindowEx is transpiled into (beautified):

export var useWindowEx = function () {
	var _a;

	// eslint-disable-next-line no-restricted-globals

	return (
		(_a = useWindow()) !== null && _a !== void 0
			? _a
			: typeof window !== 'undefined'
	)
		? window
		: undefined;
};

The original code is:

useWindow() ?? typeof window !== 'undefined' ? window : undefined

which should be executed as:

useWindow() ?? (typeof window !== 'undefined' ? window : undefined)

I suspect adding parantheses in dom.ts will fix this particular case.

Logs

No response

Requested priority

High

Products/sites affected

No response

Are you willing to submit a PR to fix?

yes

Validations

  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • The provided reproduction is a minimal reproducible example of the bug.
@ling1726
Copy link
Member

ling1726 commented Jul 8, 2024

@spmonahan you added these utilities to v8, do you think the quick fix proposed in this issue is correct?

@Jamie0
Copy link
Author

Jamie0 commented Jul 8, 2024

Turns out my Friday brain did indeed get the order of operations wrong, and come to the wrong conclusion as to why this is happening.

Nonetheless, the logic in dom.ts needs to be useWindow() ?? (typeof window != 'undefined' ? window : undefined), otherwise the return value of useWindow becomes part of the ternary condition

( I've re-titled the bug to be more appropriate :) )

@Jamie0 Jamie0 changed the title [Bug]: Nullish Coalescing Operator Incorrectly Transpiled In Published Package [Bug]: Nullish coalescing operator logic in useWindowEx is incorrect Jul 8, 2024
@spmonahan
Copy link
Contributor

Agreed that transpilation is correct and the original code is wrong. Here's a test bed to demonstrate: https://jsfiddle.net/spmonahan/wbymaq5o/38/

@Jamie0 Mind opening a PR and adding me as a reviewer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react (v8) Issues about @fluentui/react (v8)
Projects
None yet
Development

No branches or pull requests

3 participants