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

TypeScript: Better preprocessing to make importsNotUsedAsValues unnecessary #318

Closed
dummdidumm opened this issue Feb 21, 2021 · 41 comments · Fixed by #392
Closed

TypeScript: Better preprocessing to make importsNotUsedAsValues unnecessary #318

dummdidumm opened this issue Feb 21, 2021 · 41 comments · Fixed by #392
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dummdidumm
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently, TypeScript is processed on a per-file-basis. Because the script tag is only part of the truth, a transformer was written to re-add imports which to TypeScript look unused but are actually used in the template. This means that people need to be very careful about how to write their imports, which can be especially cumbersome if a type import is from the same file as a real import.

<script lang="ts">
  impor type { Type } from './foo';
  impor { Bar } from './foo';
</script>

Since TypeScript itself does not do that strict autocompletion separation (and they don't plan to change it, see this issue), people have to do it by hand.

So what would be ideal is to

  • either somehow tell TypeScript which values are used so it does not throw them out, which would make the transformer unnecessary
  • or write a more sophisticated transformer which traverses the AST and throws out all types from the imports (similar to Fix/remove type imports typescript #164, only without relying on a program if possible)

Describe the solution you'd like

Ideally, I don't have to be that strict with seperating type and value imports. I currently see two ways to get there.

Solution 1: The Svelte-AST-compile-route

This solution is similar to the one currently used in eslint-plugin-svelte3. Some logic of #155 could maybe be reused.

  1. get the whole Svelte file, blank style content
  2. transpile script tags' TS to JS using the current transform which preserves all imports
  3. do a svelte.compile which will return a list of variables with the info where they are used (module, template, etc)
  4. append all variables to the bottom of the TS code so TS sees this "fake usage" and does not throw out imports that are only used in the template
  5. transpile TS to JS without the "keep all imports" transformer

Advantages:

  • probably the most correct and safest varaint

Disadvantages:

  • Now TS needs the whole markup and would technically be a markup, not a script transformer. That's likely a breaking change
  • It wouldn't work if people use markup that needs preprocessing, or that would need to happen before the script transformation

Solution 2: Walk TS AST to find type only references

This solution would be similar to #164 . Basically, walk the AST and find all references of all imports, and filter those out who are type-only. Walking the AST without advanced capabilities like a type checker / TS program is likely tedious, but probably more robust.

Advantages:

  • no breaking change, only needs script contents

Disadvantages:

  • maybe not possible without expensive type checker (although solution 1 would also be slower)
  • there may be edge cases where this does not work: If someone imports a type in module-script but uses it as a type in the instance-script. The logic would be "oh this is unused and not otherwise used as a type, preserve it" which would be wrong.

How important is this feature to you?
Important in order to boost TypeScript development productivity.

@kaisermann
Copy link
Member

kaisermann commented Feb 21, 2021

I also prefer solution 1.

Now TS needs the whole markup and would technically be a markup, not a script transformer. That's likely a breaking change

Not necessarily! We could have a way to access the whole markup of the current file in the script/style preprocessors (I'm not sure if we would have some race condition here). If the markup needs to be preprocessed, the internal markup preprocessor could update the shared "markup" content. This would mean that the standalone typescript preprocessor should also have a markup preprocessor.

Moving ts to the markup step would be not only a big endeavour, but we would also need a way to know if we're going to deal with TS or not, complicating things even more.

Edit:

That won't work. I forgot that Svelte runs all markup preprocessors first 😭

@kaisermann kaisermann added the enhancement New feature or request label Feb 21, 2021
@dummdidumm
Copy link
Member Author

dummdidumm commented Feb 22, 2021

What if we use this knowledge and tell people if they chain multiple preprocessors and they want the better DX they need to add svelte-preprocess as last in the chain? (This would mean this DX enhancement would be optional and opt-in) Then internally svelte-preprocess could put all results of previous markup preprocessors in some shared markup results map and use that. Maybe it's also worth a shot at talking to the other maintainer about enhancing the preprocess chain a bit by passing the complete current transpilation result (whole file content), which would remove the need for keeping such a map.

Solution 2 would not work I think, I thought about this some more and there could be the situation where an import is only used as a type in the script content but used as a value within the template, marking it a false positive for removal. So only a revival of #164 would work in all cases, which would mean slower compilation.

@SomaticIT
Copy link
Contributor

Hello,

I'm investigating this issue because it's a real problem on big svelte/Typescript projects.

Maybe I can help on the implementation.

I did some tests on my local installation:

  1. Collect whole file markup (after transformation) during the markup preprocessing phase and pass it to Typescript. It avoid moving Typescript preprocessor to markup but it's a little hacky.

  2. Make svelte compiler pass the whole markup to when preprocessing (as a new option to avoid breaking change).

  3. Move Typescript to markup (I don't like this).

My preference goes to solution 2 if it's not too complex. The simplest way (but not the cleanest) is solution 1.

Do you think it's possible to add this feature in svelte compiler?

The idea of appending variables used in markup seems the only viable solution to let Typescript knows which imports it can safely remove.

The problem is that it forces to compile ts and svelte twice. It could be problematic on large projects.

Is there a safe way to extract js expressions from markup without full svelte compile (maybe ast parser)?

If possible, it could allow us to avoid the double compilation and directly append expressions at the end of the script.

@axelthat
Copy link

axelthat commented Apr 5, 2021

Any updates?

@SomaticIT
Copy link
Contributor

In my previous message, I ask two questions:

  1. Is it possible to make the svelte compiler pass the entire markup to preprocess? (using an additional property to avoid breaking changes)

  2. Is there a safe way to extract JS expressions from markup without doing a full svelte compilation?

If we can address this two questions, we can build a reasonable roadmap:

  1. svelte: Allow svelte compiler to pass full markup to preprocessors
  2. svelte-preprocess: Keep the actual process when no markup is available (to avoid breaking changes)
  3. svelte-preprocess: Extract JS expressions from markup and append them to the bottom of the script
  4. svelte-preprocess: Remove JS expressions from the built script
  5. svelte-preprocess: Remove the custom import transformer

Note: I will try to investigate on question 1 and make a PR on svelte repository.

@dummdidumm
Copy link
Member Author

I'm fairly certain question 1 can be answered with "yes"

@SomaticIT
Copy link
Contributor

I'm too, that's why I will try to open a PR on this.

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 7, 2021

I created a PR on the svelte compiler to allow it to pass the full source code to preprocessors.

This PR will allow us to avoid introducing the breaking change of making the typescript preprocessor a markup preprocessor as mentioned by @dummdidumm.

I'm still looking for a safe way to extract JS expressions from markup.

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 7, 2021

I just found that if we remove the script and style tag then run svelte.parse to extract the AST, we can have the AST of the HTML markup. It seems to be a good start but there is a lot of cases to handle.

If think that we will need to walk the AST and generate a javascript/typescript equivalent of svelte expressions.

eg:

{#each list as item (item.id)}
    <p>{item.title}</p>
{/each}

should become:

for (const item of list) {
    let var0 = item.title;
}

What do you think?

I think that this solution allows to avoid the double compilation and allow to safely extract JS expressions from svelte.
The only problem I see is that we will need to be careful when svelte add new features. eg: NewBlock will be ignored until we implement it in the extraction process.

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 7, 2021

Summary of the discussion from the svelte PR to avoid polluting the bad repo:

@dummdidumm wrote:
That sounds like too much work. The svelte compiler returns a list of variables referenced in the markup, which can be used instead

@SomaticIT wrote:
That sounds great. Do you know how can I retrieve this list?

If possible, I would like to avoid running the full compilation process since it will force us to do a double compilation and it will drastically impact the compiler performance

@SomaticIT wrote:
@dummdidumm, I tried to run the svelte compiler but the only way to make it reliable is to first compile typescript then compile svelte so the process will be very expensive...

Moreover, this way is not 100% reliable, even eslint-plugin-svelte3 is doing a full AST walk after compilation because some variables are not referenced: https://github.com/sveltejs/eslint-plugin-svelte3/blob/57ba6eca4af3df201be17d1fb7de80622ac7855e/src/preprocess.js#L147

Also, I suggest moving our discussion to the svelte-preprocess issue to avoid polluting this PR.
Do you agree?

Update: The above argument is erroneous as explained by @dummdidumm.

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 7, 2021

Regarding the expensiveness: I don't think it is that expensive since it's a per-file-compilation, the Svelte compiler does not need to look at other files to compile the given file. Moreover, if you add generate: false it will skip creating JS/CSS code which should speed it up further. The eslint plugin code you linked is needed because you can create new variables through each/await/let blocks (like the item in {#each items as item}), which is irrelevant for us, so we should be good with vars. So what I envision is:

  1. Strip scripts and styles from markup
  2. Compile with the Svelte compiler
  3. Extract vars which are referenced in the template from the result
  4. For module-script: Join module-script, instance-script, referenced variables and transpile. For instance-script: Join instance-script and referenced variables and transpile.
  5. Extract the transpile result which you care about (module-script or instance-script)

@SomaticIT
Copy link
Contributor

The eslint plugin code you linked is needed because you can create new variables through each/await/let blocks (like the item in {#each items as item}), which is irrelevant for us, so we should be good with vars.

Thank you for this clarification.


I tried running the svelte compiler on a component with stripped script and style. (It generates a lot of warnings but we can safely ignore them)

The problem is that it doesn't find the variables when there is no script.
The only variable I get in vars is a $store variable (and it doesn't see that it references the variable store and not $store).

So it seems that to get vars from the svelte compiler, we need to do the following:

  1. Transpile the code from typescript to js (with importNotUsedAsValues: "preserve")
  2. Replace the script tag content with transpiled result
  3. Compile with the Svelte compiler
  4. Extract vars and join them as you said in your comment
  5. Compile again typescript (with no importNotUsedAsValues option)
  6. Extract the compiled result

Maybe I'm wrong and I over-estimate the compilers process but it still seems very expensive to me on large projects:

  1. 2 typescript compilations per component
  2. 2 svelte compilations per component

On my (very simple) component, it takes 75ms to compile typescript (using svelte-preprocess) and 50ms to compile svelte. So if we consider that it takes the same time for the second phase, compiling this component will take 250ms to build.

On a medium/large-size project, we quickly have more than 100 components (and many of them are more complex than the one I'm testing above).

If we consider that all comonents are taking the same time to compile as above, building a 100 components project would take 25sec instead of 12.5sec.

Here is my test:

const svelte = require("svelte");
const fs = require("fs");

const component_path = "path/to/Component.svelte";
const component_source = fs.readFileSync(component_path, "utf8");

const markup = component_source
    .replace(/<!--[^]*?-->|<script(\s[^]*?)?(?:>([^]*?)<\/script>|\/>)/gi, "")
    .replace(/<!--[^]*?-->|<style(\s[^]*?)?(?:>([^]*?)<\/style>|\/>)/gi, "");

const result = svelte.compile(markup, { generate: false });
console.log(result.vars);

Am I wrong on my test or my estimations?

@SomaticIT
Copy link
Contributor

I tried to make an implementation using the svelte AST parser: repo here.

It seems to work well, imports are correctly removed and it pass all existing tests (and new ones).

This implementation does the following:

  1. Strips script and style tags from markup
  2. Parses markup to get AST
  3. Walks AST and generate code equivalent (you can find the result here)
  4. Compiles script content concatenated with the generated code
  5. Strips the generated compiled code

It seems to be a reliable implementation. However it presents some drawbacks:

  • It forces us to ensure that the code generator is updated when new svelte features are introducing changes in AST (like this one)
  • While not bad, it's not as fast as I thought: ~30ms before to compile this component, now ~50ms (on my computer)

Also, this is experimental and it misses some things:

  • Handle context="module" scripts
  • Patch sourcemap to strip the additional code
  • Optimize perfs

@dummdidumm, It could be interesting to implement the process you suggested to compare and see if the cost of performance is worth the cost of maintenance.

If you want to test it on your projects, you need to link both svelte and svelte-preprocess with local versions:

Can you please give me your reviews and feedbacks so I can see if it's interesting to continue this way?

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

why so complicated? ; )

a transformer was written to re-add imports which to TypeScript look unused but are actually used in the template.

see microsoft/TypeScript#4717 (comment)

to ensure your import is emitted in the output, use import "module"; syntax. this form of imports will not be elided.

so you can do something like:

import "myModule"; // ensure side effects are maintained regardless of the use of the module

import { myInterface } from "myModule"; // used only in type position, and will be elided along with  the import
var v: myInterface;

microsoft/TypeScript#4717 (comment) provides a proof-of-concept preprocess-ts.json (used in webpack.config.js)
ideally we would parse the TS AST to find import statements, to avoid false positives

in ts.transpileModule isolatedModules is always true

... but this is still a workaround

there is tsconfig isolatedModules (issue) which should keep unused imports
it only works with modules - to turn a non-module code fragment into a module:

Add an import, export, or an empty 'export {}' statement to make it a module.

using tsc for single file compilation:

Since TypeScript itself does not do that strict autocompletion separation (and they don't plan to change it, see this issue), people have to do it by hand.

microsoft/TypeScript#5858 (comment) says

how can i compile only one file with a tsconfig.json using tsc, inside a project with many .ts files

there is no way to do this currentlly.

but we can create a new project with only one file, no?

# bash pseudo code
mkdir /tmp/tsc-onefile
cp some-file.ts /tmp/tsc-onefile/index.ts
cp some-tsconfig.json /tmp/tsc-onefile/tsconfig.json
( cd /tmp/tsc-onefile/; tsc )

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 15, 2021

isolatedModules is already active, we need it since Svelte and the preprocessor do a per-file compilation. This means we cannot analyze/typecheck the whole graph, which means TS is unable to determine if a given import is an interface which should be removed or an actual type. So if we use "preserveImports" the build output will be wrong and throw an error at runtime ("MyInterface is not an export of X"). If we don't use it, unused imports are removed, which is also wrong since they could be referenced in the template which is not part of the script content -> this is why all this stuff is needed and additional code needs to be appended.

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 15, 2021

@milahu, the solution above is too complex I agree. We switched to the solution explained in PR #342 which uses svelte.compile.

As @dummdidumm explained, the current transformer is creating wrong build output (types are kept) and TypeScript cannot determine if an import is a value or a type in isolatedModules mode.

That's why, the best solution we found was to inject vars from template in the compiler so it could determine if imports are used as values or as types and strip them accordingly.

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

[...] per-file compilation. This means we cannot analyze/typecheck the whole graph, which means TS is unable to determine if a given import is an interface which should be removed or an actual type.

thanks for clarify

im still looking for a solution that avoids double-parsing the svelte markup ...

if we use "preserveImports" the build output will be wrong and throw an error at runtime ("MyInterface is not an export of X").

make typescript compile interfaces to "dummy exports"? (also for *.d.ts files) like

module.exports = {
  MyInterface: null,
};

also seen here. (could this cause identifier collisions?)

if we use "preserveImports"

i guess you mean { importsNotUsedAsValues: 'preserve' }
which "preserves all imports whose values are never used. This can cause imports/side-effects to be preserved."

https://devblogs.microsoft.com/typescript/announcing-typescript-3-8-beta/#type-only-imports-exports

TypeScript’s transpileModule API will emit code that doesn’t work correctly if MyThing is only a type, and TypeScript’s isolatedModules flag will warn us that it’ll be a problem. The real problem here is that there’s no way to say “no, no, I really only meant the type – this should be erased”, so import elision isn’t good enough.

As a solution in TypeScript 3.8, we’ve added a new syntax for type-only imports and exports.

import type { SomeThing } from "./some-module.js";

import type only imports declarations to be used for type annotations and declarations. It always gets fully erased, so there’s no remnant of it at runtime.

but as babel/babel#8361 (comment) says

that won't stop people from accidentally importing types using normal imports and exports and potentially breaking a build output transpiled with Babel.

@dummdidumm
Copy link
Member Author

make typescript compile interfaces to "dummy exports"? (also for *.d.ts files) like

module.exports = {
  MyInterface: null,
};

This is a no-go for me as it means messing with the user's code and not just transpiling it.

but as babel/babel#8361 (comment) says

that won't stop people from accidentally importing types using normal imports and exports and potentially breaking a build output transpiled with Babel.

That's why we need importsNotUsedAsAValues: "error"

We've been through all this thinking already, I don't see any other way than double compile, and it's not the end of the world. The first step hardly does a real compilation, it just walks the AST and extracts relevant info for the preprocessing, with generate: false, this is not in any way less performant than walking the AST yourself.

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

edit: type imports are removed -> no need for dummy exports

edit 2: something is wrong with the { importsNotUsedAsValues: 'preserve' } config
since unused values are removed -> see microsoft/TypeScript#38412 (comment)

dummy exports: are they a thought crime?

make typescript compile interfaces to "dummy exports"?

This is a no-go for me as it means messing with the user's code and not just transpiling it.

sounds like a moral argument, since technically,

could this cause identifier collisions?

can be answered with "no". in typescript, an identifier is either a type, or a value
if its a type ID, that means there is no such value ID, so its safe to add a dummy value ID
also the IDs are scoped in modules

looks like adding such a feature to typescript would make this use case much easier

new problem: we must process the imports to remove *.d.ts and rename *.ts to *.js

compilerOptions: { importsNotUsedAsValues: 'preserve' } - currently broken?
import { SomeType } from "./types"

// file extensions are preserved:
import { SomeType_ts } from "./types.ts"
import { SomeType_dts } from "./types.d.ts"

// type imports are removed:
import { SomeType_mixed, SomeInterface_mixed, SomeObject_mixed } from "./mixed"
function test(g: SomeInterface_mixed): SomeType_mixed { return g.call(SomeObject_mixed); }

let var_: SomeType
let var_ts: SomeType_ts
let var_dts: SomeType_dts

with compilerOptions: { target: 'es2015', importsNotUsedAsValues: 'preserve' } is transpiled to

import "./types";

// file extensions are preserved:
import "./types.ts";
import "./types.d.ts";

// type imports are removed:
import { SomeObject_mixed } from "./mixed";
function test(g) { return g.call(SomeObject_mixed); }

let var_;
let var_ts;
let var_dts;

... which also means i could not reproduce

if we use "preserveImports" the build output will be wrong and throw an error at runtime ("MyInterface is not an export of X").

that we see import "./types"; (etc) in the output i would consider a bug in typescript

@dummdidumm
Copy link
Member Author

Your code snippets are missing the actual problem: consider you do import { foo } from '..' which is referenced nowhere in the script content. Then TS will remove it, which is wrong if foo is referenced in the template. Also, the preserve logic which you are calling a bug is the correct behavior. The import itself is kept, but what is imported is removed. TS needs to do this because some of the imports could be interfaces which need to be removed.

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

the preserve logic which you are calling a bug is the correct behavior. The import itself is kept, but what is imported is removed. TS needs to do this because some of the imports could be interfaces which need to be removed.

sounds like we need a new ts compiler option, like { importsNotUsedAsValues: 'preserve-values' }

ts does know the difference between values and types/interfaces, and value IDs must not collide with type IDs
so a request for "dear typescript, please just remove all the typescript stuff, but dont tread on my imports" should be doable for ts

one edge-case i can think of is export { SomeType } from "./types"
where ts.transpileModule cannot see whether SomeType is a type or value
which brings us back to the slightly-dirty workaround of dummy exports

@dummdidumm
Copy link
Member Author

This won't work either, because if TS never removes anything from imports, then you need to tediously separate your interface imports from your value imports, which exactly the situation we have now and which is not nice DX. TS will merge export type { and export { from the same import location because it can determine which imports to remove - which are too many if you don't append the variables in the template, else TS wrongfully thinks that the import is unused.

All these steps of thinking you go through right now we've already been through, trust me, the best way is to double-compile + append the variables referenced in the template before compilation.

@SomaticIT
Copy link
Contributor

@milahu
I was looking for a way to avoid double compilation but the only way I found was to parse and walk the AST which is not so different as the actual solution (in terms of performance).

Also, the problem with the example you showed above is that you are in a context where TypeScript knows the entire file.

However, in Svelte, TypeScript does not know the content of the template. This leads to unexpected behaviors where TypeScript cannot determine what is really used in a Svelte file.

As mentionned by @dummdidumm, we already tried a lot of different possibilities but all of them have a lot of drawbacks (in terms of maintence, performance, complexity or responsability).

I was also against using the svelte compilation but it's the safest way to handle all cases and to ensure that it will not break in the future.

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

This won't work either, because if TS never removes anything from imports

pardon my stubbornness, but, as i said

ts does know the difference between values and types/interfaces

so ts should be able to remove all type imports
(to support the import-export case, we still would need dummy exports)

but i assume that adding this feature to typescript
would take much longer than finishing the "svelte double parse" route
so ... lets see what the future holds

@SomaticIT
Copy link
Contributor

ts does know the difference between values and types/interfaces

That's not true in isolatedModules mode.

Ref: Single-file transpilers don’t know whether someType produces a value or not [...]
Documentation: https://www.typescriptlang.org/tsconfig#isolatedModules

so ts should be able to remove all type imports

In isolatedModules mode, TS remove imports if they are not used as values inside the current module.
But since TS does not know how values are used inside Svelte template, it will also remove value imports.

sounds like we need a new ts compiler option, like { importsNotUsedAsValues: 'preserve-values' }

Here again, this compiler option could not work in isolatedModules mode since TS does not know if an import is a type or a value (it only detects how it is used inside the module but here it does not know how it is used in the template).

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

ts does know the difference between values and types/interfaces

That's not true in isolatedModules mode.

yes, i phrased that too simple. there are three cases:

1. an import is used as type (var someVar: SomeType;) so it can be safely removed
2. an import is used as value (var someVar = someValue;) so it must be preserved
3. in import is not used (or only exported), so it can be type or value

to solve case 3, we need "dummy exports":
type declarations are transpiled to dead code, and later are removed by tree-shaking

export type SomeType = number;

// ... is transpiled to ...

export const SomeType = null;

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 15, 2021

This could work but this feature should be added on Typescript side (if I'm correct).

If Typescript allows this kind of transpilation, we would be able to remove the double compilation (which is not really a double compilation since in preprocessor, it only parse and walk the AST).

The drawback here is that, in this case, we won't remove unused imports which is (imho) a great feature of Typescript.

Update:
I'm not sure that this will work with external modules.
eg: if you use a library that exports some types. They will not be available as "dummy exports" so this will break during runtime.


Instead of waiting Typescript to be enhanced, I suggest that we implement the solution in #342.

I thought that it will drastically impact performances but in fact the impact is acceptable considering the gain in DX and the minimal impact on maintenance.


If we want to mitigate the performance impact, we could use esbuild instead of Typescript (since it works pretty similarly in isolatedModules mode).

Of course, this should be an option so the developer is able to select his favorite transpiler.

What do you think?

@milahu
Copy link
Contributor

milahu commented Apr 15, 2021

The drawback here is that, in this case, we won't remove unused imports which is (imho) a great feature of Typescript.

Update:
I'm not sure that this will work with external modules.
eg: if you use a library that exports some types. They will not be available as "dummy exports" so this will break during runtime.

dang, you found the weak spot : /

tree-shaking should solve both problems, but only in production mode

development mode could work with fault-tolerant dynamic imports

var importedModule = await (async () => {
  try { return await import('./some-module.js'); }
  catch (error) { console.warn(error.message); }
  return {};
})();

these could be generated by the typescript compiler with zero cost
(only needed for the special case where ts cant say if import is type or value)

con: top level await only works in modules

@SomaticIT
Copy link
Contributor

Quick info:

I (almost) completed an implementation in #342.
I need some reviews and testers.

Could someone help?

@andrewbranch
Copy link

👋 Hey all, I work on the TypeScript team and am the primary author of the import type feature and the --importsNotUsedAsValues option.

All these steps of thinking you go through right now we've already been through, trust me, the best way is to double-compile + append the variables referenced in the template before compilation.

@dummdidumm, I admit I just stumbled onto this conversation, so hopefully you don’t mind catching me up, but have you already discussed potential solutions on the TypeScript side with the team? I wasn’t aware of these issues when working on the aforementioned features, and it feels like they are just shy of being useful for Svelte. We would definitely be open to feedback if it seems like some small tweaks could make life much easier for the Svelte ecosystem. In particular, reading this:

a transformer was written to re-add imports which to TypeScript look unused but are actually used in the template. This means that people need to be very careful about how to write their imports, which can be especially cumbersome if a type import is from the same file as a real import.

feels like we ought to be able to do better. We had a request recently, for a distinct but not totally dissimilar reason, to have a way for imported names that appear to be unused not to be elided in the emit. @dummdidumm, and anyone else interested, would you mind taking a look at microsoft/TypeScript#43393 and weighing in? Thanks to @milahu for bringing this to my attention.

@SomaticIT
Copy link
Contributor

@andrewbranch Thank you for joining this discussion. Your experience with TS compiler may be very useful here.

The main issue we are facing is that Typescript does not know what is used in the markup part of a Svelte file so it tries to strip actually used imports.

The current solution is the custom transformer mentioned above but this leads to unexpected behaviors when trying to import types and values in the same statement (types are preserved in the output which leads to runtime error).

Please note that we are using Typescript in isolatedModules mode so (if I'm correct) the compiler cannot determine if an import is a type or a value. It can only determine if it is used as value in the current module. If not, it can safely remove it.

Am I correct?
Do you know a way to workaround this?


I looked the issue you mentioned but it does not solve this issue it just enforces that we could not use the mixed import feature anymore. The only good thing I see with this solution is that it avoid the runtime error and move it to compile time...

However doubling all mixed imports is not a good idea (IMHO):

  • It adds a lot of lines in large projects
  • Existing Typescript developers are not used to this practice so the transition to Svelte is harder and frustrating for them
  • It's not well handled by language tools (they try to mix imports by default when auto-importing a module, so devs have to fix the generated code)
  • This is decreasing DX which is a core objective of Svelte team

Of course, we want the best of both worlds 😅.


I understand that the workaround we are actually developing (#342) is impacting performance and is involving a double parsing of markup but we did not find a safer way to make Typescript knows which imports it can safely remove.

I would love to find a solution that is handled directly by the Typescript compiler but I don't know how it could be safe without knowing external modules nor statements from markup. It's a lot of missing information for a compiler!

Do you have any ideas?

@andrewbranch
Copy link

Please note that we are using Typescript in isolatedModules mode so (if I'm correct) the compiler cannot determine if an import is a type or a value.

This is not quite correct, though I’m not sure it changes things for you—in isolatedModules, TypeScript hasn’t somehow lost its ability to reason about other files; it’s simply a way to indicate that you plan to use some other tool (like Babel or ts.transpileModule) which will not have the ability to reason about other files. The compiler uses this information, and its knowledge of other files, to preemptively warn you if you use a pattern which will be unsafe in single-file transpilation.

I looked the issue you mentioned but it does not solve this issue it just enforces that we could not use the mixed import feature anymore. The only good thing I see with this solution is that it avoid the runtime error and move it to compile time...

This is basically the value proposition of using TypeScript 😄

The problem with a super-magical solution where we can give you “the best of both worlds” is that it sounds extremely tailored to Svelte (and Svelte’s current preprocesing pipeline). I was hoping my proposal would be a good way to meet in the middle here, because having a mode where we remove all import type declarations and exactly preserve all other import declarations sounds totally reasonable, easy to explain, and potentially useful beyond just Svelte. Hopefully I can address some of your DX concerns—if we introduced such a mode, we would definitely add a codefix to automatically split illegal mixed imports. Also, I’m the primary maintainer of auto-imports for TS and JS—not sure if Svelte tooling uses something else on top of that, but we would of course ensure that the built-in auto-import feature respects any new compiler options.

I understand if you prefer to implement magic yourself, but thought I should jump in before a bunch of work was invested in something that we could work together on.

@dummdidumm
Copy link
Member Author

Hey @andrewbranch , thank you so much for weighing in 👋 I actually saw this issue back then and thought "this would solve our problems at authoring time" but then I saw it was closed so I thought "ok they don't want to implement that". Now I feel stupid for not having asked anyway 😄

Right now one has to split types and values "by hand" because TS always wants to merge them as soon as there's a value and a type imported from the same location. In my mind a combination of strictly separating type and value imports and always preserving imports would get us the desired output (although the latter could still be handled by a transform step in svelte-preprocess). I disagree with @SomaticIT on "this would be worse DX" because if this would be accompained by corresponding TS intellisense features for auto imports you wouldn't even realize something is different apart from (in my opinion neatly) separated type and value imports. We would get these enhancements "for free" once released because the Svelte language-server / extension is using the TS language service under the hood (not sure about the Intellij Plugin though). And we could still add some extra transform magic on our side later on if we decide this still isn't good enough.

I'll weigh in on both mentioned issues 👍

Re isolatedModules: No that does not change anything, only the Svelte file that is looked at is passed to TS for transpilation, more precisely only the code block. So if this is the file

<script lang="ts">
   const foo: string = "bar";
</script>
{foo}

Then ts.transpileModule is handed this: const foo: string = "bar" (only the script contents).

@milahu
Copy link
Contributor

milahu commented Apr 16, 2021

I looked the issue you mentioned but it does not solve this issue it just enforces that we could not use the mixed import feature anymore. The only good thing I see with this solution is that it avoid the runtime error and move it to compile time...

However doubling all mixed imports is not a good idea (IMHO):

another argument for mixed imports:

the issue can be solved by using fault-tolerant dynamic imports
so there is no need to force the user to move all type-imports to import type

just let my computer do this boring job, TS already has enough opionions ...
also TS wants to be "extensible so that external tools can use the compiler for more complex build workflows"

this workaround (dynamic imports) is only needed for development mode
in production mode, typescript will generate regular import statements for all IDs
and the build pipeline will remove unused IDs
to avoid the runtime error someType is not exported by someModule

@dummdidumm
Copy link
Member Author

Another goal of TS is to be as much in line with what regular JS outputs as possible and not introduce JS artifacts from type-only-artifacts. Replacing type imports with dummy imports is against that goal, you can't rely on people using a bundler which does tree shaking afterwards. Moreover your suggested solution involves using dynamic await which cant not be assumed to be available in all environments yet and it also changes the semantics of the generated code too much in my opinion.

@SomaticIT
Copy link
Contributor

SomaticIT commented Apr 16, 2021

@dummdidumm, @andrewbranch, I would like to share some of my thoughts. I'm not a Svelte maintainer and not part of the core team so my opinion is only based on the few projects I work on with my team. We will follow your decision.


Concerning DX, @andrewbranch solution will improve DX since we will not have to do the separation by hand. But I think DX is not only tooling, it's also readability, maintenance and keeping habits. While having tooling is a good thing, I still think that having separated imports impacts the DX because it's more difficult to understand imports for a newcomers and it impacts readability.

Our team is using TypeScript since the beginning (~10 years) so they are used to hugely leverage the type system to create strict types, mapping types, enums, etc... When working on Svelte, they have to create a lot of duplicated imports. We have some components that imports dozens of files and components. This leads to components where the import section is very huge and hard to maintain.

Example

This code:

import { method1, prop1, method2 } from "$lib/module";
import type { Type1, Type2 } from "$lib/module";

import Component1 from "$lib/components/Component1";
import type { Variant } from "$lib/components/Component1";
import Component2 from "$lib/components/Component2";
import type { Theme } from "$lib/components/Component2";
// and so on

Could be rewritten in:

import { method1, prop1, method2, Type1, Type2 } from "$lib/module";

import Component1, { Variant } from "$lib/components/Component1";
import Component2, { Theme} from "$lib/components/Component2";
// and so on
  1. This is far more readable and easier to understand for a developer
  2. It makes the component tinier
  3. It looks like what TS devs are used to see.

This is especially true when you import a lot of components and modules in the same component. The import section becomes very weird (cf. Example 2).

Real-world example

This is the import section of a page in a real-world project (using routify but applicable on sveltekit):

import { seq } from "promizr";
import { goto, url } from "@roxi/routify";
import { locale, t } from "@lib/i18n";
import type { Languages } from "@lib/i18n";
import { useMutation, useQuery, useQueryClient } from "@sveltestack/svelte-query";
import type { MutationOptions, QueryOptions } from "@sveltestack/svelte-query";
import { access, user } from "@lib/auth";
import type { Acl, Features } from "@lib/auth";
import { formatDuration } from "@lib/format";

import {
    buildApplication,
    duplicateApplication,
    getApplicationById,
    listFolders,
    restoreApplication,
    trashApplication,
    updateApplication,
} from "@lib/application";
import type { Application, Folder } from "@lib/application";

import { notify } from "@stores/notifications";
import type { Notification } from "@stores/notifications"; 
import { fileurl } from "@ui/actions/fileurl";

import Icon from "@components/icons";
import type { IconDefinition } from "@components/icons";
import Card from "@components/Card.svelte";
import Loading from "@components/Loading.svelte";
import Button from "@components/Button.svelte";
import type { Variant, Size } from "@components/Button.svelte";
import Message from "@components/Message.svelte";
import type { MessageType } from "@components/Message.svelte";
import ErrorMessage from "@components/ErrorMessage.svelte";
import HelpMessage from "@components/HelpMessage.svelte";
import HelpPopover from "@components/HelpPopover.svelte";
import type { PopoverPosition, PopoverSize } from "@components/HelpPopover.svelte";
import ContentEditable from "@components/ContentEditable.svelte";
import { Confirm, DockedDialog } from "@components/dialogs";
import type { DialogMode, DialogSize } from "@components/dialogs";

Could be rewritten in:

import { seq } from "promizr";
import { goto, url } from "@roxi/routify";
import { locale, t, Languages  } from "@lib/i18n";
import { useMutation, useQuery, useQueryClient, MutationOptions, QueryOptions } from "@sveltestack/svelte-query";
import { access, user, Acl, Features } from "@lib/auth";
import { formatDuration } from "@lib/format";

import {
    buildApplication,
    duplicateApplication,
    getApplicationById,
    listFolders,
    restoreApplication,
    trashApplication,
    updateApplication,
    Application,
    Folder,
} from "@lib/application";

import { notify, Notification } from "@stores/notifications";
import { fileurl } from "@ui/actions/fileurl";

import Icon from, { IconDefinition } "@components/icons";
import Card from "@components/Card.svelte";
import Loading from "@components/Loading.svelte";
import Button, { Variant, Size } from "@components/Button.svelte";
import Message, { MessageType } from "@components/Message.svelte";
import ErrorMessage from "@components/ErrorMessage.svelte";
import HelpMessage from "@components/HelpMessage.svelte";
import HelpPopover, { PopoverPosition, PopoverSize } from "@components/HelpPopover.svelte";
import ContentEditable from "@components/ContentEditable.svelte";
import { Confirm, DockedDialog, DialogMode, DialogSize } from "@components/dialogs";

IMHO, the second version is clearer and tinier. It's also consistent with what we have on TS side and with what a TS dev would expect to read.

Moreover, with the actual version, it's possible to use mixed imports in full Typescript files (eg in $lib). This means that, in the same project, we have two ways of writting imports, one for Svelte, one for TS. This is also something that impact DX IMHO.


I'm also concerned about the impact of this new mode on existing projects. Indeed, as explained above, it's possible to use mixed imports in Typescript files. Introducing this new mode will be a breaking change for all existing Typescript+Svelte projects.

Maybe a way to mitigate this would be to enable this new mode only in the context of Svelte files.


To summarize,

@andrewbranch solution:

Advantages:

  • Almost entirely handled by Typescript compiler
  • No more runtime errors
  • Better DX than now (but not perfect)
  • No compilation performance impact

Drawbacks:

  • Maybe a breaking change (avoided if not the default option)
  • Does not resolve the double import issue
  • Still weird for TS newcomers

@dummdidumm + @SomaticIT solution (#342):

Advantages:

  • The issue is fully resolved
  • No more errors at compile-time nor runtime
  • Optimal DX
  • Easier for TS developers
  • No breaking change

Drawbacks:

@milahu solution (fault-tolerant dynamic imports):
I will need to investigate more on this to complete the list below

Advantages:

  • Same as previous
  • + Easier to implement
  • + Less impact on the compilation performance

Disadvantages:

  • Not sure how bundlers will handle this
  • May impact in-browser performance
  • The preprocess needs to know it's in dev mode (which is not the case actually)

Maybe, the best would be to add an option to select the best mode depending on your project...

What do you think?

Also, do not hesitate to give me your feedbacks on the lists below, I will update them

@dummdidumm
Copy link
Member Author

I don't share the concern that the import list becomes unreadable. I personally never look at the list of imports anyway unless there's a hickup where I accidentally imported something with the same name but from another location.

Regarding breaking change: It would only be a breaking change for people who opt in to this new behavior by setting the corresponding flag in their tsconfig. It's a one-time tedious work but it's not rocket science. TS could even provide code action quick fixes for this, making the transition even easier.

I agree though that the solution with double parsing would result in probably the best DX with best backwards compatibility. I would like to hold off on going further with it though before getting a definitive answer if there will be something implemented on the TS side, and if so, when.

@dummdidumm
Copy link
Member Author

@andrewbranch don't want to rush you, but is there any estimate if and when we can expect the new TS compiler option? If it's discarded or will only land in about more than half a year I think we would investigate continuing the solution @SomaticIT sketched out.

@andrewbranch
Copy link

@dummdidumm I’m hoping for TypeScript 4.5. I was trying to get something into 4.4, but we decided we needed more research into exactly what Svelte’s (and Vue’s) pipelines look like and what their needs are. I’ll keep you posted, and might try to get you to chat with me and @DanielRosenwasser at some point if you’d be up for it.

@dummdidumm
Copy link
Member Author

Thanks for the update! About a possible meeting at some later time, we can do that, sure. You can hit me up through Orta, he's in the Svelte Discord, my name's the same in there as it is here.

dummdidumm pushed a commit to sveltejs/svelte that referenced this issue Jul 17, 2021
…(and mark them as warnings) (#6194)

This PR adds a new option errorMode to CompileOptions to allow continuing the compilation process when errors occured.
When set to warn, this new option will indicate to Svelte that it should log errors as warnings and continue compilation.

This allows (notably) preprocessors to compile the markup to detect vars in markup before preprocessing (in this case: script and style tags are stripped so it can produce a lot of errors).

This PR is part of a work on the svelte-preprocess side to improve the preprocessing of TypeScript files: sveltejs/svelte-preprocess#318

- allow compiler to pass error as warnings
- enforce stops after errors during compilation (for type-checking, TS doesn't know the error method throws)
- should review Element.ts:302
- added a test case for errorMode
- added documentation
@dummdidumm
Copy link
Member Author

dummdidumm commented Sep 4, 2021

Although this is fixed via the "append variables that are used in the template using Ast smarts", I'm still eagerly awaiting first class TS support for this, because our approach might not work for all cases (html not in a parseable state yet).

tomoam pushed a commit to svelte-jp/svelte-site-jp that referenced this issue Oct 30, 2021
…(and mark them as warnings) (#6194)

This PR adds a new option errorMode to CompileOptions to allow continuing the compilation process when errors occured.
When set to warn, this new option will indicate to Svelte that it should log errors as warnings and continue compilation.

This allows (notably) preprocessors to compile the markup to detect vars in markup before preprocessing (in this case: script and style tags are stripped so it can produce a lot of errors).

This PR is part of a work on the svelte-preprocess side to improve the preprocessing of TypeScript files: sveltejs/svelte-preprocess#318

- allow compiler to pass error as warnings
- enforce stops after errors during compilation (for type-checking, TS doesn't know the error method throws)
- should review Element.ts:302
- added a test case for errorMode
- added documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants