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

[feat] treat typescript namespace like import * as foo from 'foo' which refer to ImportNamespaceSpecifier in AST #3077

Closed
loynoir opened this issue Apr 21, 2023 · 7 comments

Comments

@loynoir
Copy link

loynoir commented Apr 21, 2023

Reproduce

$ npm exec -- esbuild --format=esm --bundle ./entry.mts
==> entry.mts <==
import TypeRegistry from './lib.mjs'

TypeRegistry.Clear()

==> lib.mts <==
const map = new Map<string, unknown>()

function Entries() {
  return new Map(map)
}

function Clear() {
  return map.clear()
}

export default /* @__PURE__ */{ Entries, Clear }

Actual

// lib.mts
var map = /* @__PURE__ */ new Map();
function Entries() {
  return new Map(map);
}
function Clear() {
  return map.clear();
}
var lib_default = { Entries, Clear };

// entry.mts
lib_default.Clear();

Expected

// lib.mts
var map = /* @__PURE__ */ new Map();
function Clear() {
  return map.clear();
}
var lib_default = { Clear };

// entry.mts
lib_default.Clear();

Related

sinclairzx81/typebox#401

@evanw
Copy link
Owner

evanw commented Apr 22, 2023

You also filed #3076 about a similar thing. But regardless of how you generate a JavaScript object, it's still a huge increase in complexity to tree-shake JavaScript objects in the way you are asking for, so this is not something that esbuild does.

The problem is that JavaScript objects are not namespaces, and they do not work like real namespaces do in languages which have that feature. One source of complexity is that you can still access all other properties via this. So in JavaScript it's not safe to assume that Entries can be removed if it's not referenced explicitly. And even if this isn't present in the source code, you can still later mutate the JavaScript object by overwriting one of the properties on the object with a function that uses this to get to any other property on the object.

It looks like Rollup and Parcel don't do this transformation either, which is presumably due to the same complexity issues. I recommend that you use normal ESM exports the way they are intended to be used if you are interested in enabling tree shaking of your code:

 ==> entry.mts <==
-import TypeRegistry from './lib.mjs'
+import * as TypeRegistry from './lib.mjs'
 
 TypeRegistry.Clear()
 
 ==> lib.mts <==
 const map = new Map<string, unknown>()
 
-function Entries() {
+export function Entries() {
   return new Map(map)
 }
 
-function Clear() {
+export function Clear() {
   return map.clear()
 }
 
 export default /* @__PURE__ */{ Entries, Clear }

@loynoir
Copy link
Author

loynoir commented Apr 22, 2023

@evanw

Awesome!

// lib.mts
var map = /* @__PURE__ */ new Map();
function Clear() {
  return map.clear();
}

// entry.mts
Clear();

@loynoir
Copy link
Author

loynoir commented Apr 22, 2023

@evanw

What about treat namespace like this behavior too?

https://astexplorer.net/

import * as foo from 'foo'

Ast parse it as ImportNamespaceSpecifier

But that may require library has typescript entry when bundling, because there is no namespace in js runtime.

If you think this not actionable, I'm OK to close this feat.

@loynoir loynoir changed the title [feat] tree shake default export [feat] treat namespace like import * as foo from 'foo' which refer to ImportNamespaceSpecifier in AST Apr 22, 2023
@loynoir loynoir closed this as completed Apr 22, 2023
@loynoir loynoir reopened this Apr 22, 2023
@evanw
Copy link
Owner

evanw commented Apr 22, 2023

Sorry, I don't understand what you're saying. I don't know what you mean by "namespace" or "ImportNamespaceSpecifier".

@loynoir
Copy link
Author

loynoir commented Apr 22, 2023

typescript have namespace

https://github.com/sinclairzx81/typebox/blob/376e482737bd915460f122c95c66482b77fa9416/src/typebox.ts#L781

And babel parse import * as foo from 'foo' as ImportDeclaration with specifiers ImportNamespaceSpecifier with identifier with name foo


But typescript namespace have zero tree-shake support.

And import * as foo from 'foo' already have tree-shake support, which support tree-shake dot property (add detail in case someone need to tree-shake dot property, and can reach a workaround).

@loynoir loynoir changed the title [feat] treat namespace like import * as foo from 'foo' which refer to ImportNamespaceSpecifier in AST [feat] treat typescript namespace like import * as foo from 'foo' which refer to ImportNamespaceSpecifier in AST Apr 22, 2023
@evanw
Copy link
Owner

evanw commented Apr 22, 2023

TypeScript's namespaces are just syntax sugar for constructing a mutable JavaScript object, which has all of the same issues. It would be equally unsafe for esbuild to do this for TypeScript namespaces.

One important difference between normal JavaScript objects and the objects that you get when you import a namespace (called module namespace exotic objects in the JavaScript specification) is that module objects don't allow you to mutate their properties. That means esbuild doesn't have to account for the possibilty of them being mutated.

It's the opinion of the TypeScript team that if you are already using ESM syntax, then you should probably shouldn't be using TypeScript namespaces for code organization: microsoft/TypeScript#30994 (comment). Instead you should be using ES modules. They are standardized across the JavaScript ecosystem, they have better support from existing tooling (e.g. you get tree shaking), they generate smaller code when minifying (since export names can be minified), and they even make your code run faster at run-time (since you avoid property access overhead). The TypeScript code base itself even moved away from TypeScript namespaces to ES modules because ES modules are better.

I don't think it's worth it for esbuild to add additional complexity to support these unusual and non-recommended workflows. Instead, I recommend that you do code organization using normal ESM exports the way they are intended to be used.

@loynoir
Copy link
Author

loynoir commented Apr 22, 2023

@evanw

Thanks for your very professional and very detailed explaination!

Close in favor of sinclairzx81/typebox#408

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

No branches or pull requests

2 participants