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

Add check for currentProps before calling #4247

Merged
merged 2 commits into from
Jun 8, 2018

Conversation

ZheSun88
Copy link
Contributor

@ZheSun88 ZheSun88 commented Jun 8, 2018

This is aiming to fix the client side error for vaadin/vaadin-upload-flow#51.

The upload component has a dom-repeat for its items internally. The
upload creates a change on the internal list, but with a JS component
that doesn't have a nodeID. On flow side, we don't check whether it has
a nodeID or not, but asking, which will throw an error.

After we remove the item from the component, the item is removed from
the currentProps, which becomes undefined, on flow side, we should have
checked that first, otherwise the error will be thrown again.

Test for the component will be added after this PR.


This change is Reviewable

This is aiming to fix the client side error for vaadin-upload-flow.

The upload component has a dom-repeat for its items internally. The
upload creates a change on the internal list, but with a JS component
that doesn't have a nodeID. On flow side, we don't check whether it has
a nodeID or not, but asking, which will throw an error. 

After we remove the item from the component, the item is removed from
the currentProps, which becomes undefined, on flow side, we should have
checked that first, otherwise the error will be thrown again. 

Test for the component will be added after this PR.
@denis-anisimov
Copy link
Contributor

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 351 at r1 (raw file):

currentPropsItem != undefined

This is not a really check for undefined but you don't need this check.
You need a check whether currentPropsItem has a value which should be done just as
if ( currentPropsItem && currentPropsItem.nodeId )


Comments from Reviewable

@ZheSun88
Copy link
Contributor Author

ZheSun88 commented Jun 8, 2018

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 351 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
currentPropsItem != undefined

This is not a really check for undefined but you don't need this check.
You need a check whether currentPropsItem has a value which should be done just as
if ( currentPropsItem && currentPropsItem.nodeId )

got it. Thanks.


Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 5 issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR SimpleElementBindingStrategy.java#L194: Make the enclosing method "static" or remove this set. rule
  2. MAJOR SimpleElementBindingStrategy.java#L993: Remove this unused private "invokeWhenNodeIsConstructed" method. rule
  3. MAJOR SimpleElementBindingStrategy.java#L1300: Equality tests should not be made with floating point values. rule
  4. INFO SimpleElementBindingStrategy.java#L194: Write to static field com.vaadin.client.flow.binding.SimpleElementBindingStrategy.boundNodes from instance method com.vaadin.client.flow.binding.SimpleElementBindingStrategy.bind(StateNode, Element, BinderContext) rule
  5. INFO SimpleElementBindingStrategy.java#L415: Complete the task associated to this TODO comment. rule

@denis-anisimov
Copy link
Contributor

:lgtm:


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@denis-anisimov denis-anisimov merged commit 62aa1c9 into master Jun 8, 2018
@denis-anisimov denis-anisimov deleted the add-check-for-currentProps-items branch June 8, 2018 07:35
@ZheSun88 ZheSun88 added this to the 1.0.0.rc3 milestone Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants