-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Prevented error thrown when ckeditor5-utils/src/env.ts
is used in an environment without window global object
#16370
Conversation
… is used in an environment without `window` global object.
Warning Rate Limit Exceeded@scofalik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on refining the CKEditor 5 utilities package by enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
get isMediaForcedColors() { | ||
return isMediaForcedColors(); | ||
}, |
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 first wanted to get rid of these getters and make it so that the property is resolved immediately (just like all other is*
properties above). However, the existing tests were failing and it appeared that we test whether the util reacts to the change of related settings.
Indeed, this setting can change (tested that), so I decided to keep these functions as getters.
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.
Doh... so maybe this should be a function? To indicate that it's a dynamic value?
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.
It's an API change, but from what I understand this is a freshly introduced prop and that'd be fine (not a breaking change, IMO).
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.
All the other properties of the env
object are static (should not change with time) except the two recently introduced. So it'd make sense to think twice before we commit in any direction.
packages/ckeditor5-utils/src/env.ts
Outdated
*/ | ||
export function isMotionReduced(): boolean { | ||
return window.matchMedia( '(prefers-reduced-motion)' ).matches; | ||
return globalVar.window.matchMedia ? globalVar.window.matchMedia( '(prefers-reduced-motion)' ).matches : false; |
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.
Because it was a getter, this function didn't crash in described scenario because the getter was never called. However, the getter would still crash if called in an environment without window
. To make the solution a bit more future-proof, I decided to add a check for globalVar.window
.
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 believe we should mock all methods and fields in globalVar.window
that we utilize in our codebase. Centralizing them would eliminate the need to repeat this logic in multiple places. However, perhaps this task could be deferred until later.
} ); | ||
|
||
it( 'returns false if window object is not available', () => { | ||
// `global.window` is an empty object if `window` was not available in global space. |
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.
If you have a better idea how to mock, please suggest.
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 only thing I can think of is the use of before/after() so it's properly cleaned up on a fail, but it's a simple test so I don't see this as a must-have.
packages/ckeditor5-utils/src/env.ts
Outdated
@@ -9,11 +9,13 @@ | |||
* @module utils/env | |||
*/ | |||
|
|||
import globalVar from './dom/global.js'; |
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.
import globalVar from './dom/global.js'; | |
import global from './dom/global.js'; |
I think we always use this convention.
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.
Fixed.
packages/ckeditor5-utils/src/env.ts
Outdated
*/ | ||
export function isMotionReduced(): boolean { | ||
return window.matchMedia( '(prefers-reduced-motion)' ).matches; | ||
return globalVar.window.matchMedia ? globalVar.window.matchMedia( '(prefers-reduced-motion)' ).matches : false; |
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 believe we should mock all methods and fields in globalVar.window
that we utilize in our codebase. Centralizing them would eliminate the need to repeat this logic in multiple places. However, perhaps this task could be deferred until later.
That would be a bit overkill. We are not able to get rid of DOM dependencies. You are basically asking about re-attaching whole UI and whole conversion from the DOM. That's an enormous, multi-level task (that could be titled "allow CKE5 to be run in Node"). Because, even if you mock, what next? The code must execute, you need to make alternative execution flows and scenarios, etc. I think it is enough to safeguard these functions that could be mistakenly used when they shouldn't. |
@scofalik, it seems like we didn't quite understand each other in that comment. Let's try again. I don't want to replace the entire DOM dependencies, but only provide those mocks when
Instead of returning an empty object in that catch block, we could return something like this:
This would reduce the complexity of the logic you wrote to check if that function exists. |
I understood but went too far with conclusion. Well, that could have worked for simple methods and properties, but we have a lot of |
Doh, GitHub lagged to refresh the thread for me and I started commenting in a thread: https://github.com/ckeditor/ckeditor5/pull/16370/files#r1603635378 I can see
|
Suggested merge commit message (convention)
Fix (utils): Prevented error thrown when editor files are imported in an environment without `window` global object. Closes #16368.