Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Fix types... sort of #347

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Fix types... sort of #347

merged 1 commit into from
Dec 5, 2018

Conversation

lhorie
Copy link
Contributor

@lhorie lhorie commented Dec 5, 2018

Still lots of $FlowFixMe's in tests because they are explicitly testing error messages that are triggered when arguments are of the wrong type. Most of these are inconsequential and can be ignored.

Also, lots of existential operators left because removing them is going to be highly non-trivial. The bulk of these existential operators exist as pass-through placeholders to glue the T of Token<T> to their respective plugins. Removing them will involve app.register require a generic arg, which will require codemodding apps in the wild.

One battle at a time.

Still lots of $FlowFixMe's in tests because they are explicitly testing error messages that are triggered when arguments are of the wrong type
Also, lots of existential operators left because removing them is going to be highly non-trivial
@lhorie lhorie added the bugfix label Dec 5, 2018
@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #347 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #347      +/-   ##
=========================================
+ Coverage   93.17%   93.2%   +0.02%     
=========================================
  Files          18      18              
  Lines         454     456       +2     
  Branches       89      91       +2     
=========================================
+ Hits          423     425       +2     
  Misses         17      17              
  Partials       14      14
Impacted Files Coverage Δ
src/compose.js 66.66% <ø> (ø) ⬆️
src/plugins/timing.js 95.23% <ø> (ø) ⬆️
src/tokens.js 100% <100%> (ø) ⬆️
src/client-app.js 100% <100%> (ø) ⬆️
src/plugins/ssr.js 91.17% <100%> (ø) ⬆️
src/base-app.js 90% <100%> (+0.11%) ⬆️

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 9007932...196a08e. Read the comment docs.

Copy link
Contributor

@dennisgl dennisgl left a comment

Choose a reason for hiding this comment

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

Lovely! nit: can we change the PR title before merging this 😆

@@ -13,8 +13,13 @@ import {SSRDecider} from './plugins/ssr';

import type {aliaser, cleanupFn, FusionPlugin, Token} from './types.js';

interface Register<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason we have the flow.js file was because we were unable to find a way to do method overloading. If this works, then that means we can probably get rid of that file, should we?

https://github.com/fusionjs/fusion-core/blob/master/src/base-app.js.flow#L26-L33

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 think we should keep the flow file if that's what gets used by any file other than base-app.js.flow. Internally, base-app.js still has a lot of *'s so I'd rather expose a more stable interface to its consumers

@lhorie lhorie merged commit 198d49d into master Dec 5, 2018
@AlexMSmithCA AlexMSmithCA mentioned this pull request Dec 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants