Skip to content

Fix #4747: Flow local variables #4753

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 12 commits into from
Oct 19, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Oct 17, 2017

Fixes #4747. For local variables with trailing inline comments (e.g. a = ###: number ### 1), output the comments up in the variable declarations line (var a/*: number */;). This lets Flow to work with local variables.

Branched off of #4736.

GeoffreyBooth and others added 11 commits October 5, 2017 23:38
…ntifier that's after `class`, not the variable assigned to
…wrapped in parentheses (around the comment too) so that Flow doesn't interpret it as a JavaScript label
…ock comment, output those parentheses rather than optimizing them away; this is a requirement of Flow, to distinguish from JavaScript labels
…generics

# Conflicts:
#	lib/coffeescript/nodes.js
#	src/nodes.coffee
…ments up in the variable declarations line for Flow compatibility
@GeoffreyBooth GeoffreyBooth changed the title Flow local variables Fix #4747: Flow local variables Oct 17, 2017
@GeoffreyBooth
Copy link
Collaborator Author

@thejameskyle or @xixixao or @srb- or @connec or @vendethiel or @rstormsf or @mitar or @mrmowgli or @zeekay or others: Does someone familiar with Flow mind please testing this branch? With a CoffeeScript project that uses Flow, or a project that uses one or the other that you don’t mind adapting to use both 😄 This PR should implement all remaining parts of the Flow comments-based syntax, per @thejameskyle’s notes in #4736.

@vendethiel
Copy link
Collaborator

Sorry, I never used Flow!

@jamiebuilds
Copy link
Contributor

This looks good to me :)

…local-variables

# Conflicts:
#	test/comments.coffee
@GeoffreyBooth GeoffreyBooth merged commit 0dc4755 into jashkenas:master Oct 19, 2017
@GeoffreyBooth GeoffreyBooth deleted the flow-local-variables branch October 19, 2017 13:57
@GeoffreyBooth
Copy link
Collaborator Author

Thanks so much @thejameskyle!

If you or anyone else has a “kitchen sink” file that uses all of Flow’s syntax, I’d love to write a test around it.

@jamiebuilds
Copy link
Contributor

Would you be interested in adding syntax support for Flow more than within comments?

@GeoffreyBooth
Copy link
Collaborator Author

I’ve been considering that. I think I definitely want to get the comment-based syntax fully working first, but we should discuss other options after that. Hopefully this PR might’ve already gotten us to full Flow support.

One option I’ve been considering is giving backticks the same treatment as ### comments. In CoffeeScript 1, ### comments were only allowed were expressions were allowed: you couldn’t have a ### comment! ### = 2, for example. The big comments PR (#4572) refactored comments so that ### comments could appear anywhere, like they can in JavaScript. But backticked blocks still have the same restriction as ### comments used to; you can’t write a `+ 1` = 2, for example. If we treat backticks the same way we treat ### comments, then backticked code could appear anywhere, and you could use backticks for Flow annotations the same places you’re using ### now. And best of all, this still doesn’t bake Flow into the CoffeeScript language itself; just like improving comments, all such a PR would do is improve backticks capabilities generally. I think adding anything Flow-specific into the language would be a nonstarter, but allowing backticks to function in this way would be acceptable.

@jamiebuilds
Copy link
Contributor

I think that syntax would be a great improvement over the comments. But from a code transformation perspective I'm skeptical that it would work so universally.

I'm also interested why adding a type syntax is a non-starter since JSX was added.

@GeoffreyBooth
Copy link
Collaborator Author

JSX was added in a framework neutral way: it can be used with React, with Vue, with whatever else wants to support JSX in the future. JSX support was really just adding support for XML elements as a new string type, essentially.

Explicitly supporting Flow, without a wrapper like comments or backticks, would be different. We don't want to lock the language into a particular downstream technology. It's also nowhere near as popular as JSX, or TypeScript for that matter. We might decide to try to support outputting to TypeScript in the future in some way, similar to how we're now supporting Flow.

@jamiebuilds
Copy link
Contributor

There's also the possibility that you could output to either. There's a very big overlap in syntaxes, I've been working on an AST that unifies both of them so that you could have generators to either syntax.

@GeoffreyBooth
Copy link
Collaborator Author

I would be open to considering some kind of neutral CoffeeScript syntax that could translate into either. But mine isn't the only opinion that matters. Maybe let's take this to coffeescript6/discuss#12, that's probably the thread with the most interested parties.

@xixixao
Copy link
Contributor

xixixao commented Oct 21, 2017

My opinion on the matter of static typing syntax: #4563

I don't know of a more complete summary of Flow comment syntax than https://flow.org/en/docs/types/comments/, there might be a test somewhere.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants