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

fix: inflate functions passed to __callXXXFunction methods #5080

Merged
merged 4 commits into from
Nov 29, 2022

Conversation

DiegoCardoso
Copy link
Contributor

@DiegoCardoso DiegoCardoso commented Nov 25, 2022

Description

Make the objects passed to the __callXXXFunction method go through the same "inflate" process done with the configuration object sent from the server. That is needed because objects containing properties with the _fn_ format might be passed to these functions.

Some refactoring done in the process:

  • Move the __inflateFunctions method to a helper file to make it easier to be used by both instance and static methods
  • Add some checks to inflateFunctions to make the logic to be performed only on plain objects (avoiding primitives/null/undefined and HighCharts instances that may contain circular reference)
  • Make the necessary changes on the tests

Part of vaadin/flow-components#3867

Type of change

  • Bugfix
  • Feature

Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Looks good, also tested that it works with the IT added to the Flow component 👍

Added one small comment regarding a new test.

Also target labels for cherry-picking should be set.

expect(config.tooltip.formatter).to.be.a('function');
expect(config.tooltip).to.not.have.property('_fn_formatter');
});

it('should not try to inflate if a non-object value is passed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not obvious how it is actually testing that, I guess by checking that there are no errors? Should we reflect that in the test name or in a 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.

Refactored the test to spy on Object.entries. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not so sure about it, since it relies on implementation details of the function, which can change. I don't have any better suggestions for improving the test procedure, apart from just adding a clarifying comment to your original version.

If we want to go with the spy, it should be reset afterwards since it is applied on a global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right about the spy usage. Restored the previous version and added a comment about the test.

Also removed the fixture, since it's not needed for that portion of the tests.

@sonarcloud
Copy link

sonarcloud bot commented Nov 28, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.0.0.alpha6 and is also targeting the upcoming stable 24.0.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants