🚨 Error: M(...).forEach is not a function

ampproject / error-reporting

Contains production error tracking issues.

Apache License 2.0

2 stars 5 forks source link

🚨 Error: M(...).forEach is not a function #73

Closed ampprojectbot closed 3 years ago

ampprojectbot commented 3 years ago

Details

Error report: link First seen: Feb 25, 2021 Frequency: ~ 2,636/day

Stacktrace

Error: M(...).forEach is not a function
    at forEach (extensions/amp-story/1.0/amp-story-page.js:1881:65)
    at initializeTabbableElements_ (extensions/amp-story/1.0/amp-story-page.js:367:4)
    at buildCallback (src/custom-element.js:516:30)

Notes

@Enriqe modified extensions/amp-story/1.0/amp-story-page.js:356-374 in #27367 (Mar 25, 2020)

Seen in:

03-06 Stable (0631+4)
03-12 Beta (2145+2)
03-12 Experiment-B (2145+2)
03-12 Experimental (2145+2)
03-23 Nightly (0450)
03-23 Nightly (0450+1)
+11 more

Possible assignees: @Enriqe

/cc @ampproject/release-on-duty

rcebulko commented 3 years ago

This error exceeded the Nightly error threshold but appears to be well below thresholds for other release channels. Seems like the volume is low enough to mostly ignore this

jridgewell commented 3 years ago

@Enriqe: NodeList does not have a forEach method in all browsers. Please use src/types.js's toArray, or src/dom.js's iterateCursor

dvoytenko commented 3 years ago

Can we ban NodeList.forEach and HTMLCollection.forEach in our closure conformance config?

jridgewell commented 3 years ago

It's already banned, but it's not catching.

rcebulko commented 3 years ago

Could we stub it in e2e test fixtures so any call to NodeList#forEach throws and fails the tests?

rsimha commented 3 years ago

It's already banned, but it's not catching.

Could this be because our closure warning_level is currently set to QUIET?

// TODO(amphtml): Change 'QUIET' to 'DEFAULT' after ampproject/amphtml#32875 is merged.
warning_level: argv.warning_level || 'QUIET',

Notes:

A lot of work has been done by @kristoferbaxter and @dvoytenko in https://github.com/ampproject/amphtml/pull/32875 to fix existing errors that will let us set warning_level to DEFAULT or VERBOSE, but we still need to split up and merge that PR.
Here's the reason we moved to QUIET in the first place: https://github.com/ampproject/amphtml/pull/30987

kristoferbaxter commented 3 years ago

A lot of work has been done by @kristoferbaxter and @dvoytenko in ampproject/amphtml#32875 to fix existing errors that will let us set warning_level to DEFAULT or VERBOSE, but we still need to split up and merge that PR.

I keep trying to work on splitting apart that massive PR (mostly per analytics vendor) and not having the time.

Would there be someone else who can take this on?

dvoytenko commented 3 years ago

To realistically being able to do that, I think we need to find a way to restrict errors/warnings to only src/ and extensions/ folders at first and we can expand the coverage once those are properly reported. Can we do that? If yes, I can volunteer to take on fixing errors (or reapplying fixes from #32875) to src/ and extensions/.

rsimha commented 3 years ago

We can add an option for warning_level to the type checker and surface errors only for src/ and extensions/. Should be easy since they're already checked in a single step.

Edit: Done in https://github.com/ampproject/amphtml/pull/33469.

rsimha commented 3 years ago

Would there be someone else who can take this on?

I just chatted with @samouri, who is interested in driving the merging of these fixes and the switching of the default level from QUIET to DEFAULT or better.