Skip to content

Simplify spec DOM globals assignment#5622

Merged
aduth merged 1 commit intomainfrom
aduth-simplify-js-dom-globals
Nov 19, 2021
Merged

Simplify spec DOM globals assignment#5622
aduth merged 1 commit intomainfrom
aduth-simplify-js-dom-globals

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 19, 2021

Why: To avoid needing to cherry-pick individual window properties to promote to the global namespace, which often comes as a surprise, and likely won't be clear how to resolve when the need arises. Instead, assign all window properties as global properties, which is essentially how it behaves in a browser environment.

**Why**: To avoid needing to cherry-pick individual window properties to promote to the global namespace, which often comes as a surprise, and likely won't be clear how to resolve when the need arises. Instead, assign all window properties as global properties, which is essentially how it behaves in a browser environment.
: Promise.reject(new Error('Failed to load'));
}
})(),
runScripts: 'dangerously',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that Object.getOwnPropertyNames(window) would return a different result with this assigned, and was missing many properties (e.g. window.Element). After tracking down why we needed this in the first place, I came up with the substitute implementation below, which is also similar to what we do for images.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM


const button = await findByText('doc_auth.buttons.upload_picture');
expect(button.classList.contains('usa-button--outline')).to.be.true();
expect(console).to.have.loggedError(/^Error: Could not load script:/);
Copy link
Contributor

Choose a reason for hiding this comment

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

does this not work because console is the real value and not the spy anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this not work because console is the real value and not the spy anymore?

Previously, we were letting JSDOM try to actually (virtually) load the script, and in this test scenario we were expecting that to fail, at which point JSDOM would log to the console, and we needed to allow for that to happen with loggedError.

Since we're no longer letting JSDOM load the scripts, the log no longer happens.

@aduth aduth merged commit 6b378d8 into main Nov 19, 2021
@aduth aduth deleted the aduth-simplify-js-dom-globals branch November 19, 2021 18:52
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.

2 participants