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

Update: Register both '.ts/.tsx' extensions for babel #63

Closed
wants to merge 2 commits into from

Conversation

wKich
Copy link

@wKich wKich commented Jan 13, 2020

Fix #61

@phated
Copy link
Member

phated commented Jan 13, 2020

Does this also need a new test?

@wKich
Copy link
Author

wKich commented Jan 13, 2020

I think it would be good. I'll add.

@phated
Copy link
Member

phated commented Jan 21, 2020

@wKich I've been digging into this some more and it seems that babel actually supports .ts files being compiled with JSX compilation by setting the options on "@babel/preset-typescript", like so:

{
  "presets": [
    "@babel/preset-env",
    "@babel/preset-react",
    [
      "@babel/preset-typescript",
      {
        "isTSX": true,
        "allExtensions": true
      }
    ]
  ]
}

Is there a use case for registering both extensions?

@phated
Copy link
Member

phated commented Jan 21, 2020

This is also an issue with @babel/register itself. I noticed that it "deregisters" if you try to register another extension. See https://github.com/babel/babel/blob/master/packages/babel-register/src/node.js#L98

I think the proper solution here would be to get an option upstreamed to babel. Want to give that a shot?

@phated
Copy link
Member

phated commented Jan 21, 2020

I'm going to pull in the workaround into the tests but I think we need to upstream the fix into @babel/register by adding an option to avoid de-registering.

@phated phated closed this in 0ad24f3 Jan 21, 2020
@wKich
Copy link
Author

wKich commented Jan 21, 2020

But, why not simply register both extensions? I mean, ts-node register ts/tsx extension hooks under the hood and if someone use babel instead of ts-node, it will be good to provide the same behavior.

I agree that babel de-registering is a problem in this scenario, but we can't be sure that behavior is used by somebody. Disable de-registering by default is a breaking change, adding an option increase complexity. For what?

Is there a use case for registering both extensions?

No. These options are not enable .tsx extension for babel. They are used for different purpose. From docs:

  • isTSX - Forcibly enables jsx parsing. This mean, that all regular .ts files should be parsed as .tsx
  • allExtensions - Indicates that every file should be parsed as TS or TSX And this is used for enable parse '.js' or .jsx files as .ts. Maybe it needed if your project contains .ts along side with .js files.

@wKich
Copy link
Author

wKich commented Jan 21, 2020

I'll try to create issue in babel about de-registering option.

@phated
Copy link
Member

phated commented Jan 21, 2020

Disable de-registering by default is a breaking change, adding an option increase complexity. For what?

Because this would also break the promise for .babel.js and .jsx, which I don't want to change due to this bug/problem in babel.

These options are not enable .tsx extension for babel. They are used for different purpose.

Your interpretation of the options is incorrect (they are named very poorly). I've dug into the code to understand what they actually do:

  • isTSX disables a deprecated feature of TypeScript that parsed < > brackets for a different purpose than JSX. This shouldn't be used anymore, but babel seemed to err on the side of caution.
  • allExtensions just skips a RegExp test that is done because of the isTSX decision. These should probably be on by default but I understand their caution. So it is actually more performant than splitting between .ts and .tsx files.

I'll try to create issue in babel about de-registering option.

Thank you for opening an issue. I believe that is the best way to solve the issue.

HRKings pushed a commit to HRKings/interpret that referenced this pull request Jan 15, 2022
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.

support multiple extensions for @babel/register hook
2 participants