Skip to content

Conversation

@thewilkybarkid
Copy link
Contributor

@thewilkybarkid thewilkybarkid commented Feb 7, 2022

I've wanted to add some integration between ReaderTaskEither and ReaderIO (e.g. orElseFirstReaderIOK), but doing so will rely on updating the fp-ts version.

There are a couple of changes that need to be made to support it, so I'm opening this first. I'm leaving it as a draft for comment.

Comment on lines -296 to +297
TaskOption: TaskOption<A>
TaskOptionContrib: TaskOption<A>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a collision with the fp-ts TaskOption; assume it's safe to rename?

Comment on lines 21 to 22
assert.deepStrictEqual(actual, E.right([1, 2, 3, 4]))
assert.deepStrictEqual(log, ['Executing 1', 'Executing 2', 'Executing 3', 'Executing 4'])
assert.deepStrictEqual(log, ['Executing 2', 'Executing 1', 'Executing 4', 'Executing 3'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why the order has changed in log but not actual...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing this might be related to gcanti/fp-ts#1626.

Copy link
Contributor Author

@thewilkybarkid thewilkybarkid Feb 14, 2022

Choose a reason for hiding this comment

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

My understanding is that the result will have a consistent order, but the actual order of operations won't (hence the logging appearing in an unexpected order). The log assertion shouldn't be checking the order, so a sort will get around this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sort added in 6ba8abd

{
"extends": "tslint-config-standard",
"rules": {
"deprecation": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not ideal, but there's a lot of deprecations. These can be resolved, but it's a large change (so might be better for later PRs).

@@ -1,5 +1,6 @@
/**
* @since 0.1.0
* @deprecated 0.1.27
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or release 0.2 and just remove it?

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.

1 participant