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

Allow treeShaking: false option #604

Closed
lukeed opened this issue Dec 16, 2020 · 11 comments
Closed

Allow treeShaking: false option #604

lukeed opened this issue Dec 16, 2020 · 11 comments

Comments

@lukeed
Copy link
Contributor

lukeed commented Dec 16, 2020

At least in transform and transformSync...

Ideally, with transform*, files are processed one at a time for raw translations. While most cases will benefit from treeshaking, it doesn't fit every case. For example, Svelte and Vue files will import components within a <script> block, and then the framework itself is making the connection between <script> and <tempate> scopes.

Here's an extremely simple Svelte example:

<script lang="ts">
  import Counter from './Counter.svelte';
  export let name = 'world';
</script>

<main>
  <h1>Hello {name}!</h1>
  <Counter />
</main>

When the <script> tag is processed on its own, esbuild.transform produces this output:

let name = 'world';
export {
  name
};

Consequently, this means that the rest of the Svelte component is working with an undefined Counter reference.


Because transform* is not following and/or bundling files together, IMO it's totally acceptable to leave the import Counter from './Counter.svelte'; as is – entrusting the dev knows what they're doing and/or entrusting something else to take care of that later on.,

In this particular case, Svelte and/or Rollup would pick up the Counter.svelte path in a later pass.

@jonatansberg
Copy link

I've beed down this rabbit hole before.. Setting the importsNotUsedAsValues option to preserve will leave the imports alone, but I still had problems with the names beings stripped out.

import Counter from './Counter.svelte';

Would turn in to:

import './Counter.svelte';

@lukeed
Copy link
Contributor Author

lukeed commented Dec 16, 2020

For the time being, I "fixed" this by saving & preprending the original imports to the output, while removing all imports that the output kept: lukeed/svelte-preprocess-esbuild@b1c7903 + lukeed/svelte-preprocess-esbuild@de22520

Ideally, this would still be done at esbuild's level since it actually deals with the contents... more than a RegExp, at least. Producing an accurate sourcemap is among the reasons

@evanw
Copy link
Owner

evanw commented Dec 16, 2020

I've beed down this rabbit hole before.. Setting the importsNotUsedAsValues option to preserve will leave the imports alone, but I still had problems with the names beings stripped out.

This is what the official TypeScript compiler does too, right? How do Svelte and Vue use the official TypeScript compiler?

@lukeed
Copy link
Contributor Author

lukeed commented Dec 16, 2020

TypeScript doesn't treeshake by default. When transforming a file, it will strip types and leave the transpiled JS of whatever was given.

@evanw
Copy link
Owner

evanw commented Dec 16, 2020

Here's what I used for testing:

// example.ts
import Counter from './Counter.svelte';
export let name = 'world';

Here's what you get with tsc example.ts --module esnext:

export var name = 'world';

Here's what you get with tsc example.ts --module esnext --importsNotUsedAsValues preserve:

import './Counter.svelte';
export var name = 'world';

@lukeed
Copy link
Contributor Author

lukeed commented Dec 16, 2020

Hmm.. sorry, not sure where I got that idea from then.

Would this still be something you'd consider adding? The alternatives are to trust a RegExp, or to use typescript in the end way to parse & traverse an AST.

Also, on a related note, esbuild seems to reserve files' basenames as internal variables, which then increments a variable identifier with the same name:

// input
import { foo } from './src/data';
export let value = foo;
let data = value * 3;

// esbuild output
import {foo} from "./src/data";
export let value = foo;
let data2 = value * 3; //<~ data2

// tsc output (tsc --module esnext)
import { foo } from './src/data';
export var value = foo;
var data = value * 3;

Also true with function data(){} and const data = .... Only not true with $: data = value * 3;

@jonatansberg
Copy link

@evanw svelte-preprocess seems to be using TS transformers to preserve the import statements:
https://github.com/sveltejs/svelte-preprocess/blob/main/src/transformers/typescript.ts#L35

@lukeed
Copy link
Contributor Author

lukeed commented Dec 16, 2020

Right, I checked before my last comment to make sure I wasn't missing an alternative 😆 It's an AST visitor

@evanw evanw closed this as completed in 4e25a16 Dec 31, 2020
@evanw
Copy link
Owner

evanw commented Dec 31, 2020

I'm not planning on exposing an API for AST manipulation in esbuild itself, so the same approach won't work with esbuild. And I'm not keen on adding another option for this since esbuild wasn't designed for partial module compilation so it's technically not a supported use case.

I just made a change that should hopefully make this possible in the next release. The change is described in detail in the release notes. Basically just set importsNotUsedAsValues to preserve and make sure you aren't bundling or minifying. In that specific case only, unused imports are no longer removed.

I also changed the automatically-generated import namespace variables to be prefixed with import_ which should make name collisions much less likely. This still isn't a fully robust solution because these automatically-generated names are still present, and still could occasionally collide. I didn't design esbuild with partial module compilation in mind and there could potentially be other edge cases where something breaks. But I'm guessing these edge cases probably won't be hit in practice, and I think using esbuild as a TypeScript transpiler like this should work in the vast majority of cases.

@lukeed
Copy link
Contributor Author

lukeed commented Dec 31, 2020

This is great, thank you!

@evanw
Copy link
Owner

evanw commented Oct 4, 2021

Heads up that I'm planning on changing this behavior to align with the behavior of the official TypeScript compiler. They are now releasing a flag for this called preserveValueImports and I plan to change esbuild to also require that flag for this behavior after TypeScript 4.5 is released.

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 a pull request may close this issue.

3 participants