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

Cover Grid component with types #743

Merged
merged 1 commit into from
Jul 19, 2017
Merged

Cover Grid component with types #743

merged 1 commit into from
Jul 19, 2017

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Jul 17, 2017

Wooh. It's heavy. I splitted a few types in Grid. Maybe then it will be lifted for all components. I found a lot effects with comparing undefined and number and tried to get rid in favour of -1. Will try to consolidate further.

@bvaughn
Copy link
Owner

bvaughn commented Jul 17, 2017

Yeah, Grid will probably be the worst. (Hopefully!) I'll give it a review sometime soon. 😄

@TrySound
Copy link
Contributor Author

But please don't merge anything before. I'm afraid to rebase this :)

@bvaughn
Copy link
Owner

bvaughn commented Jul 17, 2017

Absolutely.

@TrySound
Copy link
Contributor Author

How is it going?

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I gave this a really fast glance through and it seems reasonable. I trust you on this one since you seem on top of the Flow changes. 😄 Feel free to move ahead. You now have commit access.

Note there are a few open PRs. You definitely have no responsibility to review or the merge them. (Although please feel free.) I would only ask that you hold off on making Flow changes that would cause conflicts with existing PRs until I've had a chance to review and merge them. (Or you can!)

@TrySound TrySound merged commit ec0bd80 into bvaughn:master Jul 19, 2017
@TrySound TrySound deleted the grid-types branch July 19, 2017 22:47
@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

By the way @TrySound , I'm unable to run the dev server locally after pulling down master:

screen shot 2017-07-19 at 6 20 19 pm

Does the local dev server work for you? Any chance you forgot to commit a plugin/config change?

@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

Looks like CI has started failing again since this PR was merged also (eg 1522).

After doing a clean yarn install (rm -rf node_modules && yarn) I see this error locally too when running yarn build. Seems it has something to do with the babel plugin flow-react-proptypes and its interaction with one of the newly-added Grid types.. Removing that plugin suppresses the issue, but obviously isn't a real solution.

@TrySound
Copy link
Contributor Author

Branch worked fine.

@TrySound
Copy link
Contributor Author

Looks like babel fails with weird error. Trying to update deps.

@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

Hm. So you actually ran 'yarn build' in the branch, successfully? That's weird.

@TrySound
Copy link
Contributor Author

TrySound commented Jul 20, 2017

Yep. In master I got weird TypeError: source/Grid/Grid.js: Property value expected type of string but got null from babel-types used by transform-es2015-duplicate-keys. Looks like internal babel error.

@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

Hm. This error first cropped up in CI in ec0bd80 (when this branch merged to master)

@TrySound
Copy link
Contributor Author

Looks like a bug with flow react prop types. It didn't convert string keys in

type Props = {
  "read-only": string
};

It's strange. Didn't catch this error before.

@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

Interesting.

@TrySound
Copy link
Contributor Author

@bvaughn
Copy link
Owner

bvaughn commented Jul 20, 2017

Nice! 👍

@bvaughn
Copy link
Owner

bvaughn commented Jul 21, 2017

By the way, the yarn build task is now working- (thanks again! 😁) but yarn start (running the local demo site, during development) is still failing. Any idea what's going on there?

@TrySound
Copy link
Contributor Author

Tried to figure out not successful. Error message suggests to find message in node_module. I found it in ajv regenerator. Nothing changed with trace. Seems like error in one of plugins.

@TrySound
Copy link
Contributor Author

It was babel-traverse. Recommendation with trace shows only traverse call stack. Nothing is changed about a year. I don't have an idea how to fix it. But it's just a warning, not an error.

@bvaughn
Copy link
Owner

bvaughn commented Jul 21, 2017

It's actually an error (status: failed). The local dev server fails b'c of this and just serves a cached version of the site. Changes made to source files aren't reflected, even after restarting the server.

screen shot 2017-07-21 at 12 46 22 pm

@TrySound
Copy link
Contributor Author

TrySound commented Jul 23, 2017

Seems like watch mode hides errors and dashboard shows them crossing old log. I found the problem in typecheck plugin which is run with types ast before they are removed. Plugins are run before presets.(

@bvaughn
Copy link
Owner

bvaughn commented Jul 23, 2017

Thanks for tracking it down. ❤️

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