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

Synthetic custom element lifecycle swallows DOM errors for insertBefore #4319

Open
Tracked by #2964
nolanlawson opened this issue Jun 21, 2024 · 1 comment
Open
Tracked by #2964
Labels

Comments

@nolanlawson
Copy link
Collaborator

nolanlawson commented Jun 21, 2024

Description

Because of the global monkey-patching for synthetic custom element lifecycle, we actually swallow some errors that the browser would normally throw for improper usages of the insertBefore API.

This only pops up in production mode (not dev mode), because in dev mode we do other monkey-patching that also masks the issue.

Steps to Reproduce

Minimal repro (in prod mode):

const div = document.createElement('div')
const span = document.createElement('span')
div.insertBefore(span) // missing 2nd argument

This should throw:

Uncaught TypeError: Failed to execute 'insertBefore' on 'Node': 
2 arguments required, but only 1 present.

However, if the synthetic lifecycle monkey-patching is already present, then it does not throw. This is due to this code:

insertBefore(newChild, referenceNode) {
const insertedNode = insertBefore.call(this, newChild, referenceNode);
return callNodeSlot(insertedNode, ConnectingSlot);
},

If only 1 argument is supplied, then referenceNode is undefined, and we call the native insertBefore with both arguments, so no error is thrown by the browser.

Expected Results

The error should be thrown regardless of whether we're in synthetic or native custom element lifecyc.e

Actual Results

Synthetic lifecycle papers over the error.

Browsers Affected

Chrome, Firefox, and Safari all throw the same error when 1 argument is supplied to insertBefore.

@ekashida
Copy link
Member

We did .apply(this, arguments) for addEventListener() due to differences in APIs across different browsers, we should do that for all native API invocations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants