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

Box BindingData output type separate from minor types #512

Conversation

marner2
Copy link
Collaborator

@marner2 marner2 commented Sep 7, 2022

COMBINED WITH #508

Conflicts with #506 because of the moved createBinding function.

Box the output type T of BindingData separately from boxMinorTypes methods (and Binding.fs helpers). Currently, all functions in Binding.fs box it together, but this change allows for other constructs that don't box it.

Comment on lines -69 to -72
let map outMapA outMapCollection inMapA =
mapA outMapA inMapA
>> mapCollection outMapCollection

Copy link
Contributor

Choose a reason for hiding this comment

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

From #470 (comment):

I am not convinced that the changes in this file are needed yet. For example, BindingData.OneWaySeq.boxMinorTypes boxes 'a while Helpers.createBinding boxes 'aCollection. Can you change this PR so that aCollection is boxed in each binding type's boxMinorTypes function?

-- @TysonMN

When we add the 't type parameter to BindingData, we are promoting types like 'aCollection from a "minor" type to a major type. Subsequently (to maintain backwards compatibility) Helpers.createBinding boxes one of the major types of BindingData (ie, 't) so that we don't break existing code.

In one of the next PRs, we will be adding something like Helpers.createBindingT that doesn't box the major type, and then use it in the new StaticHelper class.

-- @marner2

Copy link
Member

Choose a reason for hiding this comment

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

This is less efficient and convincingly correct for you static code, but what about (just to start) you unbox 't in the future PR instead of merging this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you mean just merging #511 first? I actually split them apart so you can merge that one first.

In the end though, we need this type passed all the way through, otherwise we'll be stuck putting type annotations on the view model properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@TysonMN I am starting to do (approximately) this in #522.

@marner2 marner2 force-pushed the feature/box_bindingdata_t_separate_from_minor_types branch from d25304e to c7f4e8e Compare September 9, 2022 01:04
@TysonMN TysonMN force-pushed the feature/box_bindingdata_t_separate_from_minor_types branch from c7f4e8e to 5a6a5b3 Compare September 16, 2022 01:18
@marner2 marner2 force-pushed the feature/box_bindingdata_t_separate_from_minor_types branch 2 times, most recently from 470e2c1 to 83fd093 Compare September 16, 2022 16:03
@marner2 marner2 mentioned this pull request Sep 16, 2022
41 tasks
@marner2 marner2 force-pushed the feature/box_bindingdata_t_separate_from_minor_types branch 2 times, most recently from 3873606 to 2518e54 Compare September 23, 2022 01:55
@marner2 marner2 force-pushed the feature/box_bindingdata_t_separate_from_minor_types branch from 2518e54 to 92225a9 Compare October 2, 2022 22:28
@marner2 marner2 marked this pull request as draft March 28, 2023 19:37
@marner2
Copy link
Collaborator Author

marner2 commented Apr 5, 2023

Closed, combined with #508 to actually implement the forward-facing feature in totality.

@marner2 marner2 closed this Apr 5, 2023
@marner2 marner2 deleted the feature/box_bindingdata_t_separate_from_minor_types branch April 5, 2023 00:07
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