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.

Sign up for GitHub

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

Jump to bottom

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

Closed
leroydev opened this issue Sep 19, 2022 · 17 comments · Fixed by #1170
Closed

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

leroydev opened this issue Sep 19, 2022 · 17 comments · Fixed by #1170
Assignees
@eps1lon

Comments

@leroydev
Copy link

leroydev commented Sep 19, 2022 •
edited
Loading

@testing-library/[email protected] 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/[email protected]/dist/@testing-library/dom.esm.js

And in 8.18.0, lines 428 to 430: https://unpkg.com/browse/@testing-library/[email protected]/dist/@testing-library/dom.esm.js
@ankush ankush mentioned this issue Sep 19, 2022
Merged
@rmoralp
Copy link

rmoralp commented Sep 19, 2022 •
edited
Loading

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

Could @kentcdodds check it?

@MichaelDeBoey MichaelDeBoey mentioned this issue Sep 19, 2022
Merged
lukaszwnek added a commit to SchoolKeep/dockerfiles that referenced this issue Sep 19, 2022
@lukaszwnek
Pin the version of @testing-library/dom to fix cypress
f49921b 
testing-library/dom-testing-library#1169
@lukaszwnek lukaszwnek mentioned this issue Sep 19, 2022
Merged
ankush added a commit to resilient-tech/frappe that referenced this issue Sep 19, 2022
@ankush
build: pin testing-library/dom temporarily
40f6a34 
refer testing-library/dom-testing-library#1169
bglowacki pushed a commit to SchoolKeep/dockerfiles that referenced this issue Sep 19, 2022
@lukaszwnek @bglowacki
Pin the version of @testing-library/dom to fix cypress
a24ef8b 
testing-library/dom-testing-library#1169
mergify bot pushed a commit to frappe/frappe that referenced this issue Sep 19, 2022
@ankush @mergify
build: pin testing-library/dom temporarily
25e23cc 
refer testing-library/dom-testing-library#1169

(cherry picked from commit 40f6a34)
@sneridagh
Copy link

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

@sneridagh sneridagh mentioned this issue Sep 19, 2022
Merged
@ShaunSHamilton ShaunSHamilton mentioned this issue Sep 19, 2022
Closed
1 task
@MichaelDeBoey
Copy link
Member

@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

dom-testing-library/package.json

Line 59 in ab8182c

"kcd-scripts": "^11.0.0",

@sneridagh
Copy link

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

@MichaelDeBoey
Copy link
Member

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

@eps1lon eps1lon self-assigned this Sep 19, 2022
@eps1lon
Copy link
Member

eps1lon commented Sep 19, 2022 •
edited
Loading

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

@sneridagh
Copy link

@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

@eps1lon eps1lon mentioned this issue Sep 19, 2022
Merged
2 tasks
@sneridagh sneridagh mentioned this issue Sep 19, 2022
Closed
@MichaelDeBoey
Copy link
Member

@eps1lon Pinning kcd-scripts won't help, as we're not using latest version yet (as I described in #1169 (comment))

@eps1lon
Copy link
Member

eps1lon commented Sep 19, 2022

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
Copy link
Member

eps1lon commented Sep 19, 2022

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
Copy link

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

@sneridagh
Copy link

@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
Copy link

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

@eps1lon
Copy link
Member

eps1lon commented Sep 19, 2022

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
Copy link
Member

eps1lon commented Sep 19, 2022

🎉 This fix is included in version 8.18.1 🎉: https://unpkg.com/browse/@testing-library/[email protected]/dist/@testing-library/dom.esm.js

The release is available on:

@eps1lon eps1lon closed this as completed Sep 19, 2022
@sneridagh
Copy link

Thanks a lot @eps1lon !

@sneridagh
Copy link

I can confirm it works now without pinning.

@zburke zburke mentioned this issue Sep 20, 2022
Closed
ankush added a commit to frappe/frappe that referenced this issue Sep 20, 2022
@mergify @sagarvora @ankush
perf: cache Meta directly and other improvements (backport #18164) (#…
2de5c91 
…18181)

* perf: initialise field map when initialising meta

(cherry picked from commit df8399f)

* perf: simpler, faster meta cache

(cherry picked from commit 6c6a969)

* fix: ensure error is thrown

(cherry picked from commit b529c27)

* refactor: drop _allow_dict support

This is not required anymore since we store full doc anyway. Also since
they share same cache key calling get_cached_value and get_cached_doc
can end up doing duplicate / double work.

(cherry picked from commit abd0187)

* fix: pickle doc objects directly

This seems to be missed out and causes performance regression in IRL
usage when get_doc is called on cached doc already.

(cherry picked from commit bcb5fe9)

* test: basic perf test for rps on get_list

(cherry picked from commit c28ddcc)

* build: pin testing-library/dom temporarily

refer testing-library/dom-testing-library#1169

(cherry picked from commit 40f6a34)

* chore: semantic changes to document caching code

* fix: drop Meta cache during update

Revert "fix: drop Meta cache during update" (#18186)

* Revert "fix: drop Meta cache during update (#18182)"

This reverts commit 656f6df.

* fix: replace meta cache keys

Old keys stored different types of data `dict` changing key to indicate
change in type.

* perf: cache `FormMeta` directly (#18165)

* perf: cache `FormMeta` directly

* perf: check if `dt` is table, use `db.get_value` instead of `get_all`

Co-authored-by: Sagar Vora <[email protected]>
Co-authored-by: Ankush Menat <[email protected]>
stephenBDT pushed a commit to alias/frappe that referenced this issue Sep 30, 2022
@mergify @sagarvora
@ankush @stephenBDT
perf: cache Meta directly and other improvements (backport frappe#1…
baa351a 
…8164) (frappe#18181)

* perf: initialise field map when initialising meta

(cherry picked from commit df8399f)

* perf: simpler, faster meta cache

(cherry picked from commit 6c6a969)

* fix: ensure error is thrown

(cherry picked from commit b529c27)

* refactor: drop _allow_dict support

This is not required anymore since we store full doc anyway. Also since
they share same cache key calling get_cached_value and get_cached_doc
can end up doing duplicate / double work.

(cherry picked from commit abd0187)

* fix: pickle doc objects directly

This seems to be missed out and causes performance regression in IRL
usage when get_doc is called on cached doc already.

(cherry picked from commit bcb5fe9)

* test: basic perf test for rps on get_list

(cherry picked from commit c28ddcc)

* build: pin testing-library/dom temporarily

refer testing-library/dom-testing-library#1169

(cherry picked from commit 40f6a34)

* chore: semantic changes to document caching code

* fix: drop Meta cache during update

Revert "fix: drop Meta cache during update" (frappe#18186)

* Revert "fix: drop Meta cache during update (frappe#18182)"

This reverts commit 656f6df.

* fix: replace meta cache keys

Old keys stored different types of data `dict` changing key to indicate
change in type.

* perf: cache `FormMeta` directly (frappe#18165)

* perf: cache `FormMeta` directly

* perf: check if `dt` is table, use `db.get_value` instead of `get_all`

Co-authored-by: Sagar Vora <[email protected]>
Co-authored-by: Ankush Menat <[email protected]>
developmentforpeople pushed a commit to developmentforpeople/frappe that referenced this issue Jan 3, 2023
@mergify @sagarvora
@ankush @developmentforpeople
perf: cache Meta directly and other improvements (backport frappe#1…
10f1424 
…8164) (frappe#18181)

* perf: initialise field map when initialising meta

(cherry picked from commit df8399f)

* perf: simpler, faster meta cache

(cherry picked from commit 6c6a969)

* fix: ensure error is thrown

(cherry picked from commit b529c27)

* refactor: drop _allow_dict support

This is not required anymore since we store full doc anyway. Also since
they share same cache key calling get_cached_value and get_cached_doc
can end up doing duplicate / double work.

(cherry picked from commit abd0187)

* fix: pickle doc objects directly

This seems to be missed out and causes performance regression in IRL
usage when get_doc is called on cached doc already.

(cherry picked from commit bcb5fe9)

* test: basic perf test for rps on get_list

(cherry picked from commit c28ddcc)

* build: pin testing-library/dom temporarily

refer testing-library/dom-testing-library#1169

(cherry picked from commit 40f6a34)

* chore: semantic changes to document caching code

* fix: drop Meta cache during update

Revert "fix: drop Meta cache during update" (frappe#18186)

* Revert "fix: drop Meta cache during update (frappe#18182)"

This reverts commit 656f6df.

* fix: replace meta cache keys

Old keys stored different types of data `dict` changing key to indicate
change in type.

* perf: cache `FormMeta` directly (frappe#18165)

* perf: cache `FormMeta` directly

* perf: check if `dt` is table, use `db.get_value` instead of `get_all`

Co-authored-by: Sagar Vora <[email protected]>
Co-authored-by: Ankush Menat <[email protected]>
@nifgraup nifgraup mentioned this issue Jan 16, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eps1lon eps1lon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Pin supported target environment eps1lon/dom-testing-library
5 participants
@sneridagh @leroydev @rmoralp @MichaelDeBoey @eps1lon