[cli] Core definitions #4255
[cli] Core definitions #4255
Conversation
|
I'm thinking that a solution like this is necessary regardless to solve some of flow's problems with missing core definitions and my use case is centred around I wonder how a library that ships flow or a libdef could take advantage of core definitions. It would be very useful if a libdef of a component library was able to type it's This seems duplicative of the base definitions solution though with the major difference being that it doesn't import changes and I feel like they could co-exist solving different problems but equally could be consolidated to keep one solution for solving multiple problems. |
|
Ok after having a day to think this through here are my thoughts. Imo an environment served as a core definition and base definition are two different things, one runs globally the other is imported, redux types shouldn't be exposed globally as they are interfaced through a library for example and core definitions are a means to add more types that should be available as part of the tool depending on the application usage. To support the lib defs and flow shipped libraries respectively, I believe the first can be supported with libdefs owning a package.json just like for base definitions. If a libdef needs a core def installing it shouldn't cause conflict and won't overwrite any other core def since they are only versioned by flow version, we can pull all installed libdef's core defs and flatten the array with The second I believe automation is not necessary and a requirement on a libraries readme should be sufficient imo. |
|
I might rename everything to |
There was a problem hiding this comment.
Really excited to see this land, thanks for your hard work @Brianzchen!
I don't dig much into the implementation, overall it looks good, and the tests seem reasonable. I mostly focused on the docs and overall implementation strategy.
| @@ -0,0 +1,84 @@ | |||
| declare type jsx$HTMLElementProps = {| | |||
There was a problem hiding this comment.
Can you link to the source here?
docs/core-definitions.md
Outdated
|
|
||
| ### jsx | ||
|
|
||
| Flow is built by Meta and ships with react definitions it does not type or provide a list of available jsx properties. These can live as a core definition instead. |
There was a problem hiding this comment.
This sentence reads a little unclearly, try tweaking?
There was a problem hiding this comment.
I think it'd be worth clarify what we are lacking that we get when adding jsx as an env. Most people use Flow with React and think everything is fine, so it might be unclear why it's valuable to add it.
docs/ft-config.md
Outdated
| @@ -0,0 +1,17 @@ | |||
| # ft-config.json | |||
There was a problem hiding this comment.
How about .flow-typed-config.json? I'm not sure ft would be meaningful to most people. I don't feel strongly here, but it even took me a second to realize ft mean "flow-typed".
There was a problem hiding this comment.
What if we moved it to the root and named it flow-typed.config.json? That seems like a pretty normal standard for jest, babel, etc
|
This looks fine I'll go ahead and merge, everything is green. Edit scratch that I was reading the mobile ui wrong seems like we're still waiting on a review |
There was a problem hiding this comment.
Sorry it took so long to review! I'm really happy with this approach. 👍🏼
Before we merge this in, I'd like to do another minor deploy (if we have any pending cli changes).
And once it's merged and deployed, nothing will change for users until we write the first core definition right? In which case we can plan and deprecate accordingly.
| const ftConfig = getFtConfig(cwd); | ||
| const {env} = ftConfig ?? {}; | ||
|
|
||
| if (Array.isArray(env)) { |
There was a problem hiding this comment.
If there is no flow-typed.config.json file, none of this runs right?
There was a problem hiding this comment.
Correct, Array.isArray(env) evaluates to false. I only wrote it in this way with spread for when we extend config file in the future
Nothing will change for users until we ship a new version which is also non-breaking. No users will get this new feature unless they also implement a config in their project also. In terms of deprecation it's just that with This PR also includes the beginnings of the first real env def I plan to use extensively though I didn't want this PR that builds the CLI to have that reviewed so I added a minimal version just to prove it's functionality. Feel free to merge @gantoine when you're ready but it looks like we haven't done any more work on CLI since last 3.7.0 release. |
Implements #4250
Test Plan
ft-config.jsonexists inlibdefDir(commonlyflow-typed) it will install array of envinstallsintoflow-typed/coredir with code signatureupdatecommand worksSample usage
flow-typed.config.jsoninside project root
2. Run `flow-typed install`Installing fresh or with overwrite or updating
If it already exists and modified
If a env is referenced that does not exist or not compatible with flow version
3. Run
flow-typed outdatedIf there has been an update in the registry
If nothing compatible