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

Fix infinite recursive loop on UMD module init #505

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

Hamish-taylor
Copy link
Contributor

@Hamish-taylor Hamish-taylor commented Sep 10, 2023

This PR is to solve an issue first surfaced in issue #473.

The problem

We are assuming rg4js is loaded when document.readyState is marked as complete. However, this is not true, it's not loaded until after the load event has been triggered. According to the spec complete indicates that the load event is about to fire. This leaves a small gap where if our code is executed between the document state being marked as complete and the load event firing, an infinite recursive loop is started that will hang the application.

The fix

The current fix I have in mind for this is to add a variable to the window that is set to true after we know for sure rg4js is initialized. The UMD initialization code will then wait for this rather than document.readyState === 'complete'

src/umd.intro.js Outdated Show resolved Hide resolved
darcythomas
darcythomas previously approved these changes Sep 10, 2023
Copy link
Contributor

@darcythomas darcythomas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@xenolightning xenolightning left a comment

Choose a reason for hiding this comment

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

The change itself looks fine.

I'd like to see some tests written to express the original issue and that this resolves it.

Using the readystatechanged browser event, forcing the page into the state we see here if possible

xenolightning
xenolightning previously approved these changes Sep 12, 2023
Copy link
Contributor

@xenolightning xenolightning left a comment

Choose a reason for hiding this comment

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

Happy that the test does express the issue before the fix, and the fix does indeed resolve the race condition which can happen with UMD's

Copy link
Contributor

@darcythomas darcythomas left a comment

Choose a reason for hiding this comment

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

LGTM

@PanosNB
Copy link

PanosNB commented Sep 13, 2023

Both Darcy and Sean have looked into this and ran unit tests.

@Hamish-taylor , do at least the following before releasing: (1) presoaking a prerelease to our website (at least a day ) and (2) also try to deploy a test website that would have triggered the error to test it's ok.

Then, first release on npm and nuget; wait for a couple of days and then release on CDN. Keep CS informed on each step

@CmdrKeen FYI

@Hamish-taylor Hamish-taylor merged commit 5edd4d6 into master Sep 18, 2023
1 check passed
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.

4 participants