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

chore: remove raf polyfill #1998

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Conversation

alicialics
Copy link
Contributor

@alicialics alicialics commented Sep 30, 2023

The basics

The details

Resolves

Related to google/blockly#6937

Proposed Changes

JSDom provides a pretendToBeVisual option which will automatically polyfill raf in a NodeJS environment. For more options see:
https://github.com/jsdom/jsdom#pretending-to-be-a-visual-browser

Reason for Changes

This change will remove custom polyfill that we added in this test.

Test Coverage

I ran the affected test and made sure it passes

Documentation

N/A

Additional Information

cc @maribethb cc @BeksOmega

@alicialics alicialics requested a review from a team as a code owner September 30, 2023 23:06
@alicialics alicialics requested review from rachel-fenichel and removed request for a team September 30, 2023 23:06
@BeksOmega BeksOmega self-requested a review October 2, 2023 16:05
@BeksOmega
Copy link
Contributor

BeksOmega commented Oct 2, 2023

This looks great! Thank you for the fix @alicialics :D Could you fill out the other sections of the PR template and then I can get this merged? Even just a sentence or a N/A for each section to show that you thought about all the parts would be perfect!

[Edit: Would you be interested in fixing this in core as well? Or are you trying to stick to samples?]

@BeksOmega BeksOmega removed the request for review from rachel-fenichel October 2, 2023 16:08
@alicialics
Copy link
Contributor Author

alicialics commented Oct 2, 2023

@BeksOmega I've updated the PR description. Thanks for the suggestion! I don't see any requestAnimationFrame polyfills in https://github.com/search?q=repo%3Agoogle%2Fblockly+requestAnimationFrame&type=code

please let me know if you have an issue/code reference and I am happy to take a look.

[Edit] I just read google/blockly#6937 again and is it the goal to run blockly in nodejs environment and we do not want to rely on window?

@BeksOmega
Copy link
Contributor

[Edit] I just read google/blockly#6937 again and is it the goal to run blockly in nodejs environment and we do not want to rely on window?

Oh hm sorry I should have checked back on the issue before asking! I thought because it was filed in core we also needed some fixes in core. But I probably just filed it there because the "root cause" was the requestAnimationFrame call =)

Wrt not relying on window/webdriver for our mocha tests, I think that is something we'd like to investigate! But there are some tests that afawk do /require/ being rendered (because they deal with sizing stuff e.g.). If you could get the tests to run in node, and figure out which ones fail, I think that'd be a good place to start! (if you're interested in working on this that is)

@BeksOmega BeksOmega merged commit b0a703b into google:master Oct 2, 2023
@alicialics alicialics deleted the remove_raf_polyfill branch October 2, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants