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

Upgrade to flow 0.72 and tweak scripts #716

Merged
merged 1 commit into from
May 21, 2018
Merged

Upgrade to flow 0.72 and tweak scripts #716

merged 1 commit into from
May 21, 2018

Conversation

TrySound
Copy link
Member

Ref #712

$FlowIssue is not used
$FlowFixMe exists out of the box
flow check instead of flow won't start a server
Tweaked eslint to make it work with types and class properties

@TrySound TrySound requested a review from kof May 14, 2018 14:25
@TrySound TrySound force-pushed the upgrade-flow-72 branch 2 times, most recently from 6d78e86 to c40ebfd Compare May 14, 2018 15:47
src/RuleList.js Outdated
}

for (let index = 0; index < this.index.length; index++) {
plugins.onUpdate(name, this.index[index], sheet)
if (typeof name === 'object' || name === undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@kof I added explicit check here. You can try to test it. I don't know code base and not sure what is the problem here.

@@ -69,7 +69,7 @@ export default class PluginsRegistry {
/**
* Call `onUpdate` hooks.
*/
onUpdate(data: Object, rule: Rule, sheet: StyleSheet): void {
onUpdate(data: Object | void, rule: Rule, sheet: StyleSheet): void {
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if its a good thing to have data optional, why would someone call an update without data?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. These tests are failed if we do not allow to pass undefined.

FAILED TESTS:
  jss-default-unit
    support function values
      ‚úñ should add default unit
        Chrome 66.0.3359 (Mac OS X 10.12.6)
      Error: expected '' to equal '.a-id {\n  width: 1px;\n}'
          at Assertion.assert (webpack:///~/expect.js/index.js:96:0 <- tests/index.js:8804:13)
          at Assertion.be.Assertion.equal (webpack:///~/expect.js/index.js:216:0 <- tests/index.js:8924:10)
          at Assertion.(anonymous function) [as be] (webpack:///~/expect.js/index.js:69:0 <- tests/index.js:8777:24)
          at Context.<anonymous> (webpack:///~/jss-default-unit/tests/index.test.js:352:0 <- tests/index.js:20470:53)

  jss-nested
    function values
      ‚úñ should generate correct CSS
        Chrome 66.0.3359 (Mac OS X 10.12.6)
      Error: expected '' to equal '.a-id:hover {\n  color: red;\n}'
          at Assertion.assert (webpack:///~/expect.js/index.js:96:0 <- tests/index.js:8804:13)
          at Assertion.be.Assertion.equal (webpack:///~/expect.js/index.js:216:0 <- tests/index.js:8924:10)
          at Assertion.(anonymous function) [as be] (webpack:///~/expect.js/index.js:69:0 <- tests/index.js:8777:24)
          at Context.<anonymous> (webpack:///~/jss-nested/tests/index.test.js:470:0 <- tests/index.js:22087:45)

  jss-camel-case
    function values
      ‚úñ should generate correct CSS
        Chrome 66.0.3359 (Mac OS X 10.12.6)
      Error: expected '' to equal '.a-id {\n  font-size: 12;\n}'
          at Assertion.assert (webpack:///~/expect.js/index.js:96:0 <- tests/index.js:8804:13)
          at Assertion.be.Assertion.equal (webpack:///~/expect.js/index.js:216:0 <- tests/index.js:8924:10)
          at Assertion.(anonymous function) [as be] (webpack:///~/expect.js/index.js:69:0 <- tests/index.js:8777:24)
          at Context.<anonymous> (webpack:///~/jss-camel-case/tests/index.test.js:144:0 <- tests/index.js:19716:53)

  jss-vendor-prefixer
    prefix function values
      ‚úñ should generate correct CSS
        Chrome 66.0.3359 (Mac OS X 10.12.6)
      Error: expected '' to equal '.a-id {\n  display: flex;\n}'
          at Assertion.assert (webpack:///~/expect.js/index.js:96:0 <- tests/index.js:8804:13)
          at Assertion.be.Assertion.equal (webpack:///~/expect.js/index.js:216:0 <- tests/index.js:8924:10)
          at Assertion.(anonymous function) [as be] (webpack:///~/expect.js/index.js:69:0 <- tests/index.js:8777:24)
          at Context.<anonymous> (webpack:///~/jss-vendor-prefixer/tests/index.test.js:153:0 <- tests/index.js:22568:53)

  Integration: rules
    rule.toJSON()
      ‚úñ should handle function values
        Chrome 66.0.3359 (Mac OS X 10.12.6)
      Error: expected {} to sort of equal { color: 'red' }
          at Assertion.assert (webpack:///~/expect.js/index.js:96:0 <- tests/index.js:8804:13)
          at Assertion.eql (webpack:///~/expect.js/index.js:230:0 <- tests/index.js:8938:10)
          at Context.<anonymous> (webpack:///tests/integration/rules.js:438:45 <- tests/index.js:16268:56)

src/RuleList.js Outdated
@@ -136,13 +129,14 @@ export default class RuleList {
*/
update(name?: string | Object, data?: Object): void {
Copy link
Member

Choose a reason for hiding this comment

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

It is optional here, because we have arguments an optional first argument, which results in potentially optional second argument (now I think it was a bad idea to make optional first argument, given the flow typing)

Copy link
Member

@kof kof May 15, 2018

Choose a reason for hiding this comment

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

Is there a way to express this?

if name was passed, data is not optional, if only name is passed and its an object - it is the data object.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will look really unmaintainable. Can't we just use named arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just played with it. Seems like it's not possible to express with class method.

@kof
Copy link
Member

kof commented May 19, 2018 via email

@TrySound
Copy link
Member Author

Monorepo?)

@kof
Copy link
Member

kof commented May 19, 2018 via email

@TrySound TrySound force-pushed the upgrade-flow-72 branch 2 times, most recently from 431fc90 to f9f401a Compare May 20, 2018 21:36
@TrySound
Copy link
Member Author

@kof I achieved update types with istarkov suggestion. So can we merge then?

@kof kof merged commit 6d93f6e into master May 21, 2018
@kof
Copy link
Member

kof commented May 21, 2018

Perfect, done!

@kof kof mentioned this pull request May 21, 2018
@TrySound TrySound deleted the upgrade-flow-72 branch May 21, 2018 10:25
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