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

Replace lodash.clonedeep with structuredClone in FE #38971

Closed
wants to merge 1 commit into from

Conversation

uladzimirdev
Copy link
Contributor

Important

If you are merging into master, please add either a backport or a no-backport label to this PR. You will not be able to merge until you do this step. Refer to the section Do I need to backport this PR? in the Metabase Branching Strategy document for more details.

Warning

If that is your first contribution to Metabase, please sign the Contributor License Agreement (unless it's a tiny documentation change). Also, if you're attempting to fix a translation issue, please submit your changes to our POEditor project instead of opening a PR.

Closes https://github.com/metabase/metabase/issues/[issue_number]

Description

Describe the overall approach and the problem being solved.

How to verify

Describe the steps to verify that the changes are working as expected.

  1. New question -> Sample Dataset -> ...
  2. ...

Demo

Upload a demo video or before/after screenshots if sensible or remove the section

Checklist

  • Tests have been added/updated to cover changes in this PR

Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff e94ea96...6ff6a70.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/lib/settings/column.js
frontend/src/metabase/visualizations/visualizations/Table.unit.spec.js

Copy link

replay-io bot commented Feb 20, 2024

Status Complete ↗︎
Commit 2f57b90
Results
⚠️ 1 Flaky
2316 Passed

@uladzimirdev uladzimirdev changed the title WIP Replace lodash.clonedeep with structuredClone in FE Feb 20, 2024
constructor(...args) {
super(...args);
// https://github.com/jsdom/jsdom/issues/3363
this.global.structuredClone = cloneDeep;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how relevant/working this is, but here's a suggestion to use Node's structuredClone.
jsdom/jsdom#3363 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long story short - it broke a check that clojure uses under the hood: structuredClone(array) instanceof Array returned false

@uladzimirdev
Copy link
Contributor Author

will be replaced with #39032

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Team/QueryingComponents visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants