Skip to content

Remove use of StandardAction#82

Merged
mjarvis merged 3 commits into
ReSwift:masterfrom
hlineholm:feature/remove-standard-action
Oct 12, 2018
Merged

Remove use of StandardAction#82
mjarvis merged 3 commits into
ReSwift:masterfrom
hlineholm:feature/remove-standard-action

Conversation

@hlineholm

Copy link
Copy Markdown
Contributor

Fixes #78
Related to ReSwift/ReSwift#270

@codecov-io

codecov-io commented Oct 5, 2017

Copy link
Copy Markdown

Codecov Report

Merging #82 into master will decrease coverage by 5.97%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   80.28%   74.31%   -5.98%     
==========================================
  Files           5        5              
  Lines         208      109      -99     
==========================================
- Hits          167       81      -86     
+ Misses         41       28      -13
Impacted Files Coverage Δ
ReSwiftRouter/NavigationActions.swift 100% <ø> (+60%) ⬆️
ReSwiftRouter/Router.swift 71.42% <0%> (-11.24%) ⬇️
ReSwiftRouter/NavigationState.swift 100% <0%> (ø) ⬆️
ReSwiftRouter/NavigationReducer.swift 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e49e16...9a4dacf. Read the comment docs.

@mjarvis

mjarvis commented Oct 11, 2017

Copy link
Copy Markdown
Member

We should see about moving this conformance into an extension in a separate file, and have a cocoapod subspec for recording conformance IE pod 'ReSwift-Router/Recording' or something

@hlineholm

Copy link
Copy Markdown
Contributor Author

@mjarvis that sounds reasonable. How will we do it through Carthage? Might it be reasonable to move standard action back to ReSwift and add conformance in ReSwift-Recorder, ReSwift-Router and other future extensions and development tools built upon ReSwift?

@mjarvis

mjarvis commented Oct 16, 2017

Copy link
Copy Markdown
Member

@hlineholm I'm not certain how it would be done with carthage. Perhaps a separate framework would be needed, ReSwift-RecordableRouter or such, that provides the extensions, and has ReSwift & ReSwift-Router as dependencies?

I'm not inclined to add StandardAction back to ReSwift, as its not a core framework feature, but it does seem to make things confusing in regards to other frameworks providing Recorder capabilities...

@hlineholm

hlineholm commented Nov 26, 2017

Copy link
Copy Markdown
Contributor Author

@mjarvis FYI: I'm playing around with an action setup where I'm creating a "standard action" which u then specialise by setting the payload type. And I thought maybe this could be something we could use as a StandardAction in ReSwift. Not sure though how to make the dynamic payload type conform to CustomStringConvertible so that it can be serialized/deserialized. This might be totally unuseful but I thought I'd put it out there for anyone to discuss with me.

class Action: ReSwift.Action, CustomStringConvertible {
    var description: String {
        return type
    }
    var type: String {
        return "\(type(of: self))".replacingOccurrences(of: ".Type", with: "")
    }
}
class PayloadAction<Payload>: Action {
    let payload: Payload
    init(_ payload: Payload) {
        self.payload = payload
    }
    override var description: String {
        return "\(type)(\(payload))"
    }
}
final class CreateNode: PayloadAction<(path: ImagePath, node: Node)> {}
final class CreateNodeSuccess: PayloadAction<Node> {}
final class CreateNodeError: PayloadAction<Error> {}
final class CreateSubnode: PayloadAction<(path: ImagePath, key: ParentKey)> {}
final class CreateSubnodeSuccess: PayloadAction<Node> {}
final class CreateSubnodeError: PayloadAction<Error> {}

mjarvis referenced this pull request in ReSwift/ReSwift Mar 26, 2018
* Add performance baseline for new computer

Next commits are changes that could possibly affect these performance metrics. Updating the baselines before making the changes ensure that the changes have no noticable affect on performance.

* Add failing test for subscription mutation bug

Intentionally failing test to demonstrate a bug introduced by #325

Adding subscribers in `newState` will sometimes cause the new subscriber to be lost.

* Fix disappearing subscription bug from #325

If a new subscriber was added in `newState` caused by a state change,
assigning `subscriptions = filtered` would then discard that new
subscription.

Since `subscriptions` is a value type, we're safe to mutate in the
`forEach`, so we can fix this bug by never holding a copy of subscriptions,
instead just mutating in-place. If we changed `subscriptions` to a
reference type we would need to enumerate a copy instead.

Performance tests match baselines within allowable margins for error.
@ranhsd

ranhsd commented Apr 6, 2018

Copy link
Copy Markdown

Hi @hlineholm do you know when this pull request will be merged?

Thanks,
Ran.

@ranhsd

ranhsd commented May 22, 2018

Copy link
Copy Markdown

Hi @mjarvis Are you planning to merge it to master?

Thanks.

@DivineDominion

Copy link
Copy Markdown
Contributor

A propos serializing/deserializing the payload: what about Swift Codable conformance? Wouldn't that be feasible?

@hlineholm

hlineholm commented Oct 4, 2018

Copy link
Copy Markdown
Contributor Author

@mjarvis @DivineDominion I suggest we approve this PR and add an Issue for further discussions related to StandardAction. This is currently causing a build error.

FYI: @ranhsd

@DivineDominion DivineDominion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I ran the project tests and can confirm this does maintain integrity. I have no clue if this change is a good idea, though, since I don't use the library at all. As far as the code changes go, it's good to merge. @mjarvis Do you object to the change content-wise?

@mjarvis

mjarvis commented Oct 10, 2018

Copy link
Copy Markdown
Member

Sorry folks I've been away for a bit. I've put aside some time on Friday to go over things (including this).

@mjarvis

mjarvis commented Oct 12, 2018

Copy link
Copy Markdown
Member

I'm currently in the mindset that ReSwift-Recorder is going to be marked deprecated / unfinished, and as such, I'll merge this PR as-is, and we can shelf the re-addition of the StandardAction work to these actions in subspecs. If a user chooses to use the two of these they can easily extend the actions to conform or fork the repo themselves.

@mjarvis mjarvis merged commit 9a368ff into ReSwift:master Oct 12, 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.

5 participants