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

Add esbuild-register for typescript extensions #77

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

hasparus
Copy link
Contributor

Hi @phated 👋

I felt so smart with my workaround until you mentioned we could do this in a civilized way :P

@hasparus
Copy link
Contributor Author

image

Test for .wisp is failing on my machine. Is this just on my side? Did I break it with my changes somehow?

@phated
Copy link
Member

phated commented Mar 31, 2022

Thanks! Wisp is super old and I've always had problems with it. Don't worry about it here

@hasparus
Copy link
Contributor Author

Thanks! Wisp is super old and I've always had problems with it. Don't worry about it here

All right 👌 LMK if there's something to change in my code then.

index.js Outdated
@@ -146,6 +146,7 @@ var extensions = {
'typescript-register',
'typescript-require',
'sucrase/register/ts',
'esbuild-register',
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this hooks too many extensions if you use it the "default" way: https://github.com/egoist/esbuild-register/blob/master/src/node.ts#L67-L74

I believe we need to implement this similar to babel/register. See their register function at https://github.com/egoist/esbuild-register/blob/master/src/node.ts#L96

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense — so we can pass just .ts or .tsx to extensions.

https://github.com/egoist/esbuild-register/blob/master/src/node.ts#L98

Copy link
Member

Choose a reason for hiding this comment

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

Yep! We needed to add the "extended" config syntax so we didn't hook extra extensions with babel. We want to be really delicate with the hooking

Copy link
Contributor Author

@hasparus hasparus Mar 31, 2022

Choose a reason for hiding this comment

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

Ouch, we'll have to require esbuild-register/dist/node, right? It's not exposed to the outside.

Something like the following?

    {
      module: 'esbuild-register/dist/node',
      register: function(mod) {
        mod.register({
          extensions: '[.ts'],
          target: 'node' + process.version.slice(1),
        });
      },
    },

I'm not a huge fan of accessing /dist/node , but we also can't use esbuild directly, so maybe it
ould be better to create an issue asking if dist/node can be considered public API.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with your snippet, but I also agree that it should probably be part of their public API. Not sure if that is something @egoist would want to do.

@hasparus
Copy link
Contributor Author

Hey @phated, I just pushed the change with "extended" config syntax.

image

I'm not sure if it's all right, because now the require-xml test failed. Could I have broken it or is it a flaky test?

@phated
Copy link
Member

phated commented Mar 31, 2022

Just flakey. We might just drop support for these extensions in a breaking release 🤔

Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! 🙇

@phated phated merged commit 963f5fa into gulpjs:master Apr 1, 2022
@hasparus
Copy link
Contributor Author

hasparus commented Apr 2, 2022

Hey, I just remembered one thing — I didn't update the README to show this change in #API section, and neither did HRKings with swc in #74.

@phated
Copy link
Member

phated commented Apr 2, 2022

Thanks! I'm actually hoping to finish #36 before we ship v3!

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