Skip to content

Deep clone config#4851

Closed
thomasloven wants to merge 1 commit into
devfrom
deep-clone-config
Closed

Deep clone config#4851
thomasloven wants to merge 1 commit into
devfrom
deep-clone-config

Conversation

@thomasloven
Copy link
Copy Markdown
Contributor

Proposed change

#4509 undid #2496.
Custom cards sometimes change their configuration. This makes sure that change does not propagate back to the global lovelace config.
This also protects against changes caused by bugs in core cards.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

try {
// @ts-ignore
element.setConfig(config);
element.setConfig(deepClone(config));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't do this. This hurts our performance because cards are doing the incorrect thing.

Instead we should use Object.freeze and let custom cards crash so they fix their bad behavior. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should freeze it as soon as we load the config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Object.freeze() is shallow, so that would be a recursive process. But I guess it's better to do it once...
Skärmavbild 2020-02-12 kl  08 45 53

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we should deep freeze. Deep freeze example code is on the docs I linked :) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze#What_is_shallow_freeze

Copy link
Copy Markdown
Contributor Author

@thomasloven thomasloven Feb 13, 2020

Choose a reason for hiding this comment

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

🤦‍♂ 🙈

@basnijholt
Copy link
Copy Markdown
Contributor

I assume this fixes problems like thomasloven/lovelace-template-entity-row#14?

@thomasloven
Copy link
Copy Markdown
Contributor Author

Yes, but I'll fix that the correct way too.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 13, 2020

Closing this in favor of #4862

@balloob balloob closed this Feb 13, 2020
@lock lock Bot locked and limited conversation to collaborators Feb 14, 2020
@pvizeli pvizeli deleted the deep-clone-config branch April 9, 2020 08:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants