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

Update sanctuary-type-identifiers to version 3 #425

Merged
merged 1 commit into from
May 14, 2020

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented May 5, 2020

This update has been a long time coming. I'll explain the reason for waiting so long below, alongside the relevant diff. This update is possibly a breaking change, but I don't see yet how. Maybe @davidchambers can think of a scenario where it would break someone's code. :)

Closes #418 by supersession.

@Avaq Avaq requested a review from davidchambers May 5, 2020 09:41
@Avaq Avaq self-assigned this May 5, 2020
@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #425 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #425   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         1976      1987   +11     
=========================================
+ Hits          1976      1987   +11     
Impacted Files Coverage Δ
src/future.js 100.00% <100.00%> (ø)
src/par.js 100.00% <100.00%> (ø)

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 83b0894...87a3eda. Read the comment docs.

@Avaq
Copy link
Member Author

Avaq commented May 5, 2020

I just realized how this is a breaking change: The removed Future['@@type'] property was used as a public interface by some future-* libs.

@Avaq Avaq added the breaking label May 5, 2020
src/future.js Show resolved Hide resolved
@Avaq Avaq force-pushed the avaq/sanctuary-type-identifiers branch from 49220f8 to d3c2a25 Compare May 5, 2020 12:20
@Avaq Avaq removed the breaking label May 5, 2020
@Avaq
Copy link
Member Author

Avaq commented May 5, 2020

This PR is no longer breaking because @davidchambers came up with a way to preserve compliance with sanctuary-type-identifiers versions 1 and 2 without interfering with sanctuary-type-identifiers 3: d3c2a25#diff-f9b600f4388a4ec2c76b0b6648a13b44R62-R67

@Avaq Avaq force-pushed the avaq/sanctuary-type-identifiers branch from d3c2a25 to 4f05f05 Compare May 5, 2020 13:22
src/future.js Outdated Show resolved Hide resolved
src/par.js Outdated Show resolved Hide resolved
src/future.js Outdated
Comment on lines 63 to 65
// In order to prevent sanctuary-type-identifiers version 3 from identifying
// Future as a member of $$type, we ensure that Future.constructor.prototype
// is equal to Future.
Copy link
Contributor

Choose a reason for hiding this comment

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

$$type is not of type Type but of type String. 🤪

I think $$type should be replaced with Future a b in the comment above.

The comment could begin with To prevent rather than In order to prevent without any loss of meaning.

Copy link
Member Author

@Avaq Avaq May 5, 2020

Choose a reason for hiding this comment

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

I don't like bringing the type parameters into the picture, because they have nothing to do with sanctuary-type-identifers. I rephrased that part though. What do you think of the current phrasing?

@Avaq Avaq force-pushed the avaq/sanctuary-type-identifiers branch from 6f014ed to d7b7e14 Compare May 5, 2020 17:15
@Avaq Avaq force-pushed the avaq/sanctuary-type-identifiers branch from d7b7e14 to 87a3eda Compare May 5, 2020 17:33
@Avaq Avaq merged commit ae30639 into master May 14, 2020
@Avaq Avaq deleted the avaq/sanctuary-type-identifiers branch May 14, 2020 15:47
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.

2 participants