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

Fix handling of different JSX pragmas in Typescript #7878

Merged
merged 4 commits into from May 14, 2018

Conversation

@calebeby
Copy link
Contributor

calebeby commented May 7, 2018

Q                       A
Fixed Issues? Fixes #7631
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

Before, if a JSX pragma was imported in typescript, the import was removed because it did not think it was used as a value. This changes it so it knows that the import is being used as a JSX pragma

calebeby added 2 commits May 7, 2018
Fixes #7631

Before, if a JSX pragma was imported, the import was removed because
it did not think it was used as a value. This changes it so it knows
that the import is being used as a JSX pragma
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented May 7, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7918/

@calebeby calebeby changed the title Fix handling of different JSX pragmas Fix handling of different JSX pragmas in Typescript May 7, 2018
@hzoo
hzoo approved these changes May 8, 2018
Copy link
Member

hzoo left a comment

Hmm @DanielRosenwasser?

Still makes me want a preset that just includes all of what TS supports but I guess you have to pass down options

Copy link

DanielRosenwasser left a comment

Looks reasonable to me, @weswigham and @andy-ms might want to take a quick glance though.

@DanielRosenwasser

This comment has been minimized.

Copy link

DanielRosenwasser commented May 8, 2018

Wait, I just realized this is not a global option applied to all transforms. I'll try to get back to this.

@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 8, 2018

Yeah, like @hzoo mentioned in #7631, we could figure out some way for the ts plugin to read the config from the jsx plugin, but at this point, I think that adding an option to the ts plugin is the best solution, since babel doesn't have anything in place afaik to retrieve configuration from other plugins.

@hzoo maybe in ~a month when I'm out of school I can help with making a ts preset that has a single jsx pragma option that it passes to both the ts plugin and the jsx plugin

@andy-ms
andy-ms approved these changes May 9, 2018
@xtuc
xtuc approved these changes May 9, 2018
if (binding.identifier.name != "React") {
if (
binding.identifier.name !== "React" &&
binding.identifier.name !== options.jsxPragma

This comment has been minimized.

Copy link
@xtuc

xtuc May 9, 2018

Member

what about using "React" as default in options.jsxPragam, that would simplify the logic a bit.

This comment has been minimized.

Copy link
@calebeby

calebeby May 9, 2018

Author Contributor

✔️

@xtuc
xtuc approved these changes May 9, 2018
@weswigham

This comment has been minimized.

Copy link

weswigham commented May 9, 2018

TS also supports the @jsx pragma to specify the factory inline that the jsx transform supports - it probably has a similar elision bug.

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented May 9, 2018

That's a good point, how could we keep both in sync?

At the moment it's an option but that's probably not the easiest way to configure it. I mean the pragma will be better.

@weswigham

This comment has been minimized.

Copy link

weswigham commented May 9, 2018

IMO, extract inline pragma detection out of the jsx plugin, then call it in both plugins.

@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 9, 2018

@weswigham can you elaborate a bit? I can understand extracting the logic that finds the jsx pragma from the @jsx comment, but do you think we can somehow extract the option for the pragma? How?

@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 9, 2018

Also, how does Babel plugin ordering work? I really don't understand this, because if we could just get the jsx plugin to run before this, we wouldn't have to deal with this.

@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 9, 2018

Also we should deal with fragments 😞

@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 9, 2018

Import referenced as value -> keep
Import is jsx pragma by options or by @jsx comment -> keep
Import is jsx fragment pragma by options or by @jsxFragment comment -> keep
Otherwise, remove the import

I don't think it is necessary to check if the pragma is used, because if an import is the jsx pragma, it must not be a type, it is a value.

I think we need to come up with a way for the typescript plugin to talk to the jsx plugin, and it should ask what the jsx pragma and jsx fragment pragma are.

Is there a way we can attach additional data along with the ast, which the jsx plugin can write to and the ts plugin can read from?

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented May 11, 2018

Plugin ordering is atm http://new.babeljs.io/docs/en/next/plugins.html#plugin-preset-ordering (plugins first, then presets). I think we can land as is with the caveat of ordering and then we need to figure out how to pass data in between.

Yeah we can just do the same options for now but should think about how config data should be shared (or like i said before if the typescript preset should include all of these anyway plugins + options which would solve it too I believe)

…sx-pragma-import
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented May 14, 2018

We can just land this as is since it fixes the bug

IMO, extract inline pragma detection out of the jsx plugin, then call it in both plugins.

But yeah we should probably do this or figure out a better fix

@hzoo hzoo merged commit 25810d2 into babel:master May 14, 2018
3 checks passed
3 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@calebeby

This comment has been minimized.

Copy link
Contributor Author

calebeby commented May 14, 2018

@calebeby calebeby deleted the calebeby:fix-ts-jsx-pragma-import branch May 26, 2018
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.