salesforce / lwc

⚡️ LWC - A Blazing Fast, Enterprise-Grade Web Components Foundation
https://lwc.dev
Other
1.64k stars 393 forks source link

Synthetic custom element lifecycle swallows DOM errors for `insertBefore` #4319

Open nolanlawson opened 4 months ago

nolanlawson commented 4 months ago

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:

https://github.com/salesforce/lwc/blob/7b0418c79e4159c5a8d0f8bda5ded0f1cf312ce7/packages/%40lwc/engine-dom/src/apis/create-element.ts#L68-L71

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 commented 4 months ago

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