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

Move component props from stash to stash.component_props #626

Merged
merged 4 commits into from Mar 16, 2018

Conversation

samj1912
Copy link
Collaborator

No description provided.

@samj1912 samj1912 changed the title Move componenet props from stash stash.component_props Move component props from stash to stash.component_props Mar 11, 2018
@samj1912 samj1912 force-pushed the portstash branch 3 times, most recently from d4eaa02 to 9b3104d Compare March 11, 2018 08:09
@samj1912 samj1912 force-pushed the portstash branch 2 times, most recently from 772327c to 0ac7a80 Compare March 12, 2018 06:37
current_view => 'Node',
component_path => 'isrc/Index.js',
component_props => {
isrcs => $isrcs,
Copy link
Member

Choose a reason for hiding this comment

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

Ok, they're already in the stash so normally I would say not to serialize them twice, but the isrcs should be a small list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I remove them from the stash earlier on then?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, perhaps. I thought other actions might be using them since they are set in _load, but it looks like this is the only action in this controller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, which is why I didn't touch it.

current_view => 'Node',
component_path => 'instrument/List',
component_props => {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unsatisfactory to create broken commits like this - if someone bisecting lands on this commit, the page will be broken for them because the component isn't changed to understand this until b137a99 from what I can tell.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

instrument_types: $ReadOnlyArray<InstrumentTypeT>,
instruments_by_type: {|
+[number]: $ReadOnlyArray<InstrumentT>,
unknown: $ReadOnlyArray<InstrumentT>,
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 now missing the + on all the props expect [number] (did you say some eslint rule was removing those? we should disable it)

Making PropsT an exact object type would be nice too: {| ... |}

@mwiencek mwiencek merged commit 55b50de into metabrainz:master Mar 16, 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
2 participants