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

Fixes for CanJS 4 #22

Merged
merged 6 commits into from
Nov 12, 2018
Merged

Fixes for CanJS 4 #22

merged 6 commits into from
Nov 12, 2018

Conversation

christopherjbaker
Copy link
Contributor

@christopherjbaker christopherjbaker commented Nov 7, 2018

resolves #20, continues #21

  • Update tests to test inserting the components on the page instead of using ReactTestUtils (thanks @ivospinheiro)
  • on createComponent ref callback get the component viewModel from the element using can-symbol can.viewModel an assign the react props to viewModel (thanks @ivospinheiro)
  • remove can-define from tests; use can-component builtins instead
  • use can-reflect to modify viewmodel instance

using ReactTestUtils
- on createComponent ref callback get the component viewModel from the
element using `can-symbol` `can.viewModel` an assign the react props to
viewModel
using ReactTestUtils
- on createComponent ref callback get the component viewModel from the
element using `can-symbol` `can.viewModel` an assign the react props to
viewModel
using ReactTestUtils
- on createComponent ref callback get the component viewModel from the
element using `can-symbol` `can.viewModel` an assign the react props to
viewModel
using ReactTestUtils
- on createComponent ref callback get the component viewModel from the
element using `can-symbol` `can.viewModel` an assign the react props to
viewModel
using ReactTestUtils
- on createComponent ref callback get the component viewModel from the
element using `can-symbol` `can.viewModel` an assign the react props to
viewModel
@christopherjbaker
Copy link
Contributor Author

@ivospinheiro I brought your code into a local branch so we can continue iterating on it. Your fix is what was needed, though I altered it to use can-reflect.assign instead of just vm.assign, and I took DefineMap out of the tests, since the can4 automatically turns the definitions into viewmodel instances (though the SimpleMap instances do not have .assign methods, hence the switch to can-reflect). Your -issues repo runs successfully with this code.

At present, this will mount the component without any data (causing the initial render), then immediately assign the props to the VM, causing a second render with the correct data. I am still looking into

@ivospinheiro
Copy link
Collaborator

At present, this will mount the component without any data (causing the initial render), then immediately assign the props to the VM, causing a second render with the correct data. I am still looking into

@christopherjbaker I've noticed that but I had no idea how to fix that. What I understood was that the component was being rendered but with no initial data. Is there any way to set set the bound properties when creating the element on render function. Because the ref call will only be done after the component has been already rendered, I guess?

@ivospinheiro
Copy link
Collaborator

Hi @christopherjbaker and @justinbmeyer.
Can this pull request be accepted and can-react-component be released as it is and open a new issue to track the double rendering of the component?
Another question, is this package officially part of CanJS 4/5, because it is not listed on canjs.com website?

@christopherjbaker christopherjbaker merged commit 17cf380 into master Nov 12, 2018
@christopherjbaker christopherjbaker deleted the dev/20-canjs4 branch November 12, 2018 23:26
@christopherjbaker christopherjbaker mentioned this pull request Nov 12, 2018
@christopherjbaker
Copy link
Contributor Author

Released as v2.0.0, and opened #23 to address the issue of double rendering at mount.

@ivospinheiro
Copy link
Collaborator

@christopherjbaker many thanks.
This fix will unlock the upgrade from canJS@3 to canJS@5 that I'm working in.

@christopherjbaker
Copy link
Contributor Author

Glad to be of service!

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.

can-react-component does not work as expected with CanJS 4
2 participants