testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.26k stars 467 forks source link

Nullish coalescing operator (??) in dist in 8.18.0 #1169

Closed leroydev closed 1 year ago

leroydev commented 1 year ago

@testing-library/dom@8.18.0 has a nullish coalescing operator in the distributed build in the file dist/@testing-library/dom.esm.js. This results in only Node.js 14 and up being able to execute this code and in errors in Cypress 9.1.0: image

Check here, 8.17.1, lines 409 to 413: https://unpkg.com/browse/@testing-library/dom@8.17.1/dist/@testing-library/dom.esm.js

And in 8.18.0, lines 428 to 430: https://unpkg.com/browse/@testing-library/dom@8.18.0/dist/@testing-library/dom.esm.js

rmoralp commented 1 year ago

It seems to be related to last kcd-scripts updates.

Could @kentcdodds check it?

sneridagh commented 1 year ago

@kentcdodds This is quite important, probably breaking the build of a lot of people...

MichaelDeBoey commented 1 year ago

@rmoralp @sneridagh It can't be because the latest changes in kcd-scripts as @testing-library/dom is still using v11 & kcd-scripts is already at v12.3

https://github.com/testing-library/dom-testing-library/blob/ab8182cfc5164f5d0b0fea143cfde350369488c9/package.json#L59

sneridagh commented 1 year ago

It's because of Cypress... no kcd-scripts involved...

MichaelDeBoey commented 1 year ago

Since I can't assign this ticket to a team, I'll ping @testing-library/dom to look into this

eps1lon commented 1 year ago

I'm on it. Will pin kcd-scripts. Future updates to tooling need a more thorough audit

sneridagh commented 1 year ago

@MichaelDeBoey @eps1lon The culprit is in @testing-library/cypress since it has @testing-library/dom as a dependency with caret:

https://github.com/testing-library/cypress-testing-library/blob/main/package.json#L44

MichaelDeBoey commented 1 year ago

@eps1lon Pinning kcd-scripts won't help, as we're not using latest version yet (as I described in https://github.com/testing-library/dom-testing-library/issues/1169#issuecomment-1251329766)

eps1lon commented 1 year ago

The culprit is in @testing-library/cypress since it has @testing-library/dom as a dependency with caret:

That is not the culprit. The culprit is that we introduced syntax (nullish coalescing) in a minor release. However, we still support Node.js 12 which does not support nullish coalescing. The minor release should've been a major release with a proper changelog (drop support for Node.js 12).

eps1lon commented 1 year ago

Pinning kcd-scripts won't help, as we're not using latest version yet

We need to pin some build dependency. But we do need to pin since previous releases did produce correct builds. But since we have no lockfile, a new build used newer build tooling that no longer compiled away nullish coalescing. kcd-scripts is the start point of that depencency tree that needs to be pinned since it's responsible for bundling.

sneridagh commented 1 year ago

@eps1lon +1, I meant my culprit, what caused my projects to fail :)

sneridagh commented 1 year ago

@eps1lon I double checked, and our builds run on 16:

https://github.com/plone/ploneconf.org/actions/runs/3080420601/jobs/4979205093#step:7:138

And failed anyways... clueless about what could it be.

For reference: https://github.com/plone/ploneconf.org/actions/runs/3080420601/jobs/4979205093#step:7:138

sneridagh commented 1 year ago

oh, of course it does not run on Node... is webpack cypress who tries to load it.

eps1lon commented 1 year ago

We previously always transpiled targetting whatever Browserlist returned in its default preset. However, this is a pretty weak API contract to have. I'm pinning the supported versions to the ones returned by the default preset used in 8.17.x (last known good release) + Node.js 12 (since kcd-scripts mistakes the module entrypoint as the one for browsers).

eps1lon commented 1 year ago

:tada: This fix is included in version 8.18.1 :tada:: https://unpkg.com/browse/@testing-library/dom@8.18.1/dist/@testing-library/dom.esm.js

The release is available on:

sneridagh commented 1 year ago

Thanks a lot @eps1lon !

sneridagh commented 1 year ago

I can confirm it works now without pinning.