Better Typescript transpilation #392

dummdidumm · 2021-08-22T10:48:01Z

Before submitting the PR, please make sure you do the following

It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

Run the tests with npm test or yarn test

This PR builds upon the stellar work by @SomaticIT in #318 . See that PR for more info on the "why do this"?

This PR superseeds #318 and adds the following additions:

moves new functionality behind a flag. Dynamically imports required new libraries. This makes the change backwards compatible
adds a check and test for the case where TS code consists of types only and has no content after transpilation
merge main

Closes #318

- inject vars from markup to typescript - do not use importTransform when markup is available - inject component script to context=module script - refactor modules/markup to reuse common functions - test injection behavior

- introduce two deps: magic-string, sorcery - produce a source map on each step of the process - store all source maps in a chain - refactor transformer to increase readability

…into typescript-imports

dummdidumm · 2021-08-22T10:56:51Z

The failing tests seem unrelated, I guess they appear as a cause of the Svelte dev dependency bump. Edit: Yes, this PR sveltejs/svelte#5854 added support for inline source maps and notes that they are removed from the output while merging the source maps, so the way the tests test this no longer works.

I also noticed that build will no longer work without "skipLibCheck": true in the tsconfig.json because of some type errors in the latest Svelte version.

src/types/options.ts

Inline source maps are stripped in recents Svelte preprocess versions, therefore we need to test the source map generation differently

kaisermann · 2021-08-23T11:52:38Z

Neat! I've been a bit AFK these last two months, so I'll need to get my bearings regarding this whole scenario again, but it looks promising!

In time: what's the relation between #342 and this PR? Can we close #342?

dummdidumm · 2021-08-23T11:57:47Z

In time: what's the relation between #342 and this PR? Can we close #342?

Somewhat related. If the direction of adding a new option is accepted, then yes #342 can be closed. If we want to make this new transpilation the new default behavior, we first need to change the implementation and that could happen in #342 as well. For my arguments in favor of making this an option, see the comment above.

test/fixtures/TypeScriptImports.svelte

SomaticIT · 2021-08-23T12:34:00Z

Hi @dummdidumm,

Thank you for taking some time on this PR. Your evolution is great.

Here is my feedback:

I think that both sorcery and magic-string could be added as dependencies (or bundled). They seems to be small and could be used by others transformers (like replace). Moreover, I think that forcing people to install dependencies manually is a source of errors.
I'm not sure that we need to introduce a new option. During my tests, I tried this new preprocessor on a big project and it doesn't create any breaking changes. Maybe we could take some time to investigate on more projects to be sure but I think it's safe.
We could improve the preprocessor performance by using the new advanced transpiler only when running in dev mode. When compiling for production, rollup will automatically remove unused imports so this will generates a runtime issue. It forces us to create a new PR on svelte to pass the dev option to preprocessors.

So I have 3 questions:

Could we include sorcery and magic-string as dependencies (or bundle them)?
Could we safely remove the option?
Could we improve the performance by running this transpiler only in dev mode?

dummdidumm · 2021-08-23T12:39:59Z

1. I think that both `sorcery` and `magic-string` could be added as dependencies (or bundled). They seems to be small and could be used by others transformers (like `replace`). Moreover, I think that forcing people to install dependencies manually is a source of errors.

They already have to for typescript, so the mode of adding new dependencies should be familiar.

2. I'm not sure that we need to introduce a new option. During my tests, I tried this new preprocessor on a big project and it doesn't create any breaking changes. Maybe we could take some time to investigate on more projects to be sure but I think it's safe.

I trief this, too, and didn't find any breaking things, so I'm also very confident that this works. The argument for making it an option is not related to this for me.

3. We could improve the preprocessor performance by using the new _advanced_ transpiler only when running in `dev` mode. When compiling for production, `rollup` will automatically remove unused imports so this will generates a runtime issue. **It forces us to create a new PR on `svelte` to pass the `dev` option to preprocessors.**

I think we shouldn't make these assumptions. Preprocessors could run in other contexts unrelated to Rollup or without removing unused imports as well.

My arguments in favor of the option:

backwards compatible. If this is the new default, then either it's a breaking change because people would need to install sorcery and magic-string, or we add them to dependencies but then everyone, even those who don't use TypeScript, need to download the additional packages
this is somewhat of an intermediate solution: there are plans from the TypeScript team to provide a new import option which would strictly separate types and values, which would make this advanced transpilation unnecessary. If that solution is implemented, people are free to choose the solution right here or use the new TypeScript option.

SomaticIT · 2021-08-23T12:50:20Z

They already have to for typescript, so the mode of adding new dependencies should be familiar.

You're right, but it means that you now need to install 3 additionnal dependencies (typescript, magic-string, sorcery). Installing typescript is necessary when you use typescript in a project so it's normal to install it manually. However installing magic-string and sorcery is more obscure.

backwards compatible. If this is the new default, then either it's a breaking change because people would need to install sorcery and magic-string, or we add them to dependencies but then everyone, even those who don't use TypeScript, need to download the additional packages

I'm in favor of adding them to dependencies. In this comment, I explain that both dependencies can be reused to improve the sourcemap handling of svelte-preprocess. (magic-string for replace and sorcery to combine multiple transformers sourcemaps).

this is somewhat of an intermediate solution: there are plans from the TypeScript team to provide a new import option which would strictly separate types and values, which would make this advanced transpilation unnecessary. If that solution is implemented, people are free to choose the solution right here or use the new TypeScript option.

I'm not totally sure that this will be an intermediate solution. Even if TypeScript provides a new import option to strictly separate types and values, this will generate additional code and will make imports less readable. Those who are using TypeScript for long time are used to combine types and values in the same import statement and IMHO, they will prefer staying like this.

dummdidumm · 2021-08-23T13:40:15Z

backwards compatible. If this is the new default, then either it's a breaking change because people would need to install sorcery and magic-string, or we add them to dependencies but then everyone, even those who don't use TypeScript, need to download the additional packages

I'm in favor of adding them to dependencies. In this comment, I explain that both dependencies can be reused to improve the sourcemap handling of svelte-preprocess. (magic-string for replace and sorcery to combine multiple transformers sourcemaps).

In the case of magic-string that's certainly true, it could be used in other scenarios. sorcery - not so much, as combining multiple sourcemaps is already handled by Svelte core. That said, the functionality from Svelte core could maybe be exported to be used.

this is somewhat of an intermediate solution: there are plans from the TypeScript team to provide a new import option which would strictly separate types and values, which would make this advanced transpilation unnecessary. If that solution is implemented, people are free to choose the solution right here or use the new TypeScript option.

I'm not totally sure that this will be an intermediate solution. Even if TypeScript provides a new import option to strictly separate types and values, this will generate additional code and will make imports less readable. Those who are using TypeScript for long time are used to combine types and values in the same import statement and IMHO, they will prefer staying like this.

I get that sentiment. We should rename the option to something that doesn't sound like it's superior to the current solution, like handleMixedImports. I still vote that it's an option. If we make it the default, then there needs to be an additional check for the minimum Svelte version installed in the repository, or everything is surrounded with try-catch, which could get expensive performance-wise.

@kaisermann you asked about the TypeScript PR, it looks like it might land in 4.5: microsoft/TypeScript#44619

kaisermann · 2021-08-23T14:28:21Z

However installing magic-string and sorcery is more obscure.

Agreed, seems too specific and obscure. Also, I've been wanting to use magic-string in the replace transformer for a while now, so adding it as a dependency feels natural.

We should rename the option to something that doesn't sound like it's superior to the current solution, like handleMixedImports

Maybe allowMixedImports? Anyway, I like this naming direction a bit more than useAdvanced..., which seems too generic (advanced how?).

If we make it the default, then there needs to be an additional check for the minimum Svelte version installed in the repository

This wouldn't be the first time we did this: https://github.com/sveltejs/svelte-preprocess/blob/6346194254/src/autoProcess.js#L92.

We can have it now as an opt-in feature and wait for a bit to see how it fairs in the wild and set it as the default behaviour on a future minor or possibly major version.

dummdidumm · 2021-08-23T14:40:52Z

I changed the option name to handleMixedImports (allowMixedImports is just as good for me, you decide) and made sorcery and magic-string "real" dependencies.
Last question is whether or not to make this the default. Since the don't have the dependency problem anymore and since we can check for the Svelte version, I'm slightly in favor of making it the new default for everyone who has the appropriate minimum Svelte version. Edit: The most recent commit implements the check for the Svelte version and enables the new transpilation mode if possible by default.

@SomaticIT could you give this one more try on your own project to see if that I didn't break anything? I think this is good to merge now if there are no objections to the option name.

SomaticIT · 2021-08-23T16:46:10Z

It seems good for me.
I will give it a try on a large project and give you my feedback.
Hope I have time tomorrow.

src/transformers/typescript.ts

only syntax errors are reported, not semantic errors

dummdidumm · 2021-08-30T15:43:42Z

@SomaticIT were you able to have a look? Would love to get this merged. @kaisermann any change request for the PR or is this good to go for you?

kaisermann · 2021-08-30T18:11:35Z

I will give the transformer a final look, format the file and we should be good. I should be able to do this in the following days 👍

SomaticIT · 2021-09-01T11:13:58Z

Victory!!! 🎉

I finally found what was causing the comment issue, it was my fault 😥.
Indeed, when I extracted the tag regex from transformMarkup to createTagRegex in src/modules/markup.ts, I forgot the first /.

This was making markup transform to handle comments as <template> tag.

Now everything is working fine on my project, I have no regression and I can merge type and value imports 💪!

@dummdidumm, you can find the fix in the last commit of the PR I submitted on your fork.
Or you can find the patch here: fix-markup-tag-regex.patch

dummdidumm · 2021-09-01T11:36:05Z

Thank you for looking into this! I found one more edge case I fixed: It's valid to import something in the instance script but only use it in the module script. Therefore I append the module script below the instance script now. @kaisermann this is now good to go (the lintings should be fine now, too), please make sure that the "co-authored by"-message for @SomaticIT is part of the squash-merge-commit so he gets proper credit.

SomaticIT · 2021-09-01T18:10:43Z

I updated my local version and everything is working fine!
Congratulations for your work!

I was happy to collaborate with you on this very exciting feature!!

SomaticIT · 2021-09-03T10:17:16Z

@dummdidumm, I found one more edge case: the codestores detection regex can detect reserved keywords.
I fixed it by filtering reserved keywords from the result.

Patch

You can find the patch here: code-vars-filter-reserved-keywords.patch

Exemple:

const filter = { $in: ["test"] };

In the above code, the codestores regex will detect $in as a potential store so it will append in to the injected vars but in is a reserved keyword in javascript. It is safe to remove it since it's not possible to create a store with a reserved keyword.

Edit 1:

I updated the patch above because some reserved keywords were removed in ES5/6.
If you already downloaded it, please use the latest version.

FYI, the reserved keyword list comes from this page: https://www.w3schools.com/js/js_reserved.asp
Maybe we should mention it in comments in the getJavascriptReservedKeywords function.

kaisermann

Just added some minor comments. Other than that this looks great, good job @SomaticIT and @dummdidumm 🎉

src/transformers/typescript.ts

SomaticIT · 2021-09-03T12:44:21Z

It seems good for me, I'll continue to use this specific version on my largest project to see if I discover more edge cases. However it seems to work well for me.

kaisermann · 2021-09-04T12:54:51Z

Released as v4.9.0 🎉 Awesome job 🙏

janosh · 2021-10-01T16:37:41Z

moves new functionality behind a flag

Are you referring to "useAdvancedImportTranspiler": true? Where does this go? Putting it in tsconfig.json gives me

Unknown compiler option 'useAdvancedImportTranspiler'

on latest [email protected].

dummdidumm · 2021-10-01T17:54:37Z

It's called handleMixedImports. sveltePreprocess({typescript: {handleMixedImports: true}}) in your svelte.config.js . Note that it's automatically enabled if you have at least Svelte 3.39

janosh · 2021-10-04T09:54:14Z

@dummdidumm Looks like this change does not apply to imports from '@sveltejs/kit' inside .svelte files.

// src/routes/index.svelte
import type { LoadInput, LoadOutput } from '@sveltejs/kit' // works
import { LoadInput, LoadOutput } from '@sveltejs/kit' // errors

Failed to resolve entry for package "@sveltejs/kit". The package may have incorrect main/module/exports specified in its package.json: Missing "." export in "@sveltejs/kit" package

Is this expected?

See sveltejs/kit#2384 (comment) for details.

dummdidumm · 2021-10-04T16:26:53Z

This is expected. If your import consists of types only, you need to use import type . If there's at least one value import, you can now mix them using import , which wasn't possible previously.

janosh · 2021-10-04T16:36:15Z

I see. Prob should have read this thread more carefully.

TrySound · 2021-10-13T18:11:28Z

package.json

    "ts-jest": "^25.1.0",
    "typescript": "^3.9.5"
  },
  "dependencies": {
    "@types/pug": "^2.0.4",
    "@types/sass": "^1.16.0",
    "detect-indent": "^6.0.0",
+    "magic-string": "^0.25.7",
+    "sorcery": "^0.10.0",


sorcery was not supported for a long time and adds a few unnecessary dependencies.

Got a good alternative to achieve source map concatenation?

I could bump sorcery to node 10 or 12 and get rid from "sander" package. Though I see svelte-preprocess still supports node 9. Do you plan major bump in near future?

Does this mean you have write access to the repo? I only see Rich having contributed to that repository. Can't speak for how soon @kaisermann plans on bumping, but if a little cleanup can be done I have nothing against that. Not pressing for me since the solution seems to work so far.

I don't yet but I can ask Rich

I could bump sorcery to node 10 or 12 and get rid from "sander" package. Though I see svelte-preprocess still supports node 9. Do you plan major bump in near future?

Yeah, I've been wanting to remove the deprecated things and do a little cleanup.

Since we're already here, I've started a discussion about one of the things on my mind for the next major: #424

SomaticIT and others added 6 commits April 14, 2021 12:39

feat: pass source markup to transformers (if provided)

12c1157

feat: allow typescript to know used variables in template

e5a73db

- inject vars from markup to typescript - do not use importTransform when markup is available - inject component script to context=module script - refactor modules/markup to reuse common functions - test injection behavior

fix: handle sourcemaps in typescript transformer

8c97341

- introduce two deps: magic-string, sorcery - produce a source map on each step of the process - store all source maps in a chain - refactor transformer to increase readability

Merge remote-tracking branch 'somaticit/@feature/typescript-imports' …

18c3d13

…into typescript-imports

bump svelte for passing tests, handle empty code/map case

31f6a50

move new transpilation into a flag for backwards compatibility

d588307

lint, make build work

81f9aec

dummdidumm commented Aug 23, 2021

View reviewed changes

src/types/options.ts Outdated Show resolved Hide resolved

Simon Holthausen added 2 commits August 23, 2021 11:18

Adjust failing tests

e8f1e07

Inline source maps are stripped in recents Svelte preprocess versions, therefore we need to test the source map generation differently

docs

e6f7b22

kaisermann reviewed Aug 23, 2021

View reviewed changes

test/fixtures/TypeScriptImports.svelte Show resolved Hide resolved

Simon Holthausen added 2 commits August 23, 2021 16:30

rename option, more docs

2b0ea9e

Remove dynamic imports of source-map and sorcery, make them "real" deps

ca73a4e

Simon Holthausen added 2 commits August 23, 2021 16:52

make handling mixed imports the default

856a98e

adjust docs

78db9eb

dummdidumm commented Aug 24, 2021

View reviewed changes

src/transformers/typescript.ts Outdated Show resolved Hide resolved

dummdidumm added 2 commits August 24, 2021 12:44

Ignore possible errors in $$vars$$

d349c64

undo ts-ignore as it's not needed

cec5391

only syntax errors are reported, not semantic errors

SomaticIT mentioned this pull request Sep 1, 2021

Everything is working :) dummdidumm/svelte-preprocess#1

Closed

Simon Holthausen added 4 commits September 1, 2021 13:20

handle imports in instance script that are only used in module script

ca66810

remove unused code

b041847

fix comment regression + add test for it

23c24f6

lint

de8e077

kaisermann approved these changes Sep 3, 2021

View reviewed changes

src/transformers/typescript.ts Outdated Show resolved Hide resolved

src/transformers/typescript.ts Outdated Show resolved Hide resolved

src/transformers/typescript.ts Outdated Show resolved Hide resolved

Simon Holthausen added 2 commits September 3, 2021 13:19

filter out reserved keywords to prevent syntax errors

3d561aa

cleanup

fe37ccb

kaisermann approved these changes Sep 4, 2021

View reviewed changes

kaisermann merged commit a1c54fc into sveltejs:main Sep 4, 2021

bluwy mentioned this pull request Sep 4, 2021

svelte-kit, typescript, error: "The requested module does not provide an export name" #340

Closed

timostamm mentioned this pull request Sep 21, 2021

Using with Svelte-Kit, error: "The requested module does not provide an export name" timostamm/protobuf-ts#94

Closed

janosh added a commit to janosh/awesome-sveltekit that referenced this pull request Oct 1, 2021

drop importsNotUsedAsValues thanks to sveltejs/svelte-preprocess#392

3e149e6

dummdidumm deleted the typescript-imports branch October 1, 2021 17:37

TrySound reviewed Oct 13, 2021

View reviewed changes

dummdidumm mentioned this pull request Jun 4, 2022

Preprocessing with Babel makes svelte components imports to be dropped #521

Closed

l1bbcsg mentioned this pull request Dec 21, 2022

Typescript in component markup #568

Closed

AlexErrant added a commit to AlexErrant/Pentive that referenced this pull request May 30, 2023

drop importsNotUsedAsValues thanks to sveltejs/svelte-preprocess#392

b7db1dd

manuel139 added a commit to manuel139/awesome-sveltekit that referenced this pull request Sep 4, 2024

drop importsNotUsedAsValues thanks to sveltejs/svelte-preprocess#392

5baf9f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better Typescript transpilation #392

Better Typescript transpilation #392

dummdidumm commented Aug 22, 2021 •

edited

Loading

dummdidumm commented Aug 22, 2021 •

edited

Loading

kaisermann commented Aug 23, 2021

dummdidumm commented Aug 23, 2021

SomaticIT commented Aug 23, 2021 •

edited

Loading

dummdidumm commented Aug 23, 2021

SomaticIT commented Aug 23, 2021

dummdidumm commented Aug 23, 2021 •

edited

Loading

kaisermann commented Aug 23, 2021

dummdidumm commented Aug 23, 2021 •

edited

Loading

SomaticIT commented Aug 23, 2021

dummdidumm commented Aug 30, 2021

kaisermann commented Aug 30, 2021

SomaticIT commented Sep 1, 2021 •

edited

Loading

dummdidumm commented Sep 1, 2021

SomaticIT commented Sep 1, 2021

SomaticIT commented Sep 3, 2021 •

edited

Loading

kaisermann left a comment

SomaticIT commented Sep 3, 2021

kaisermann commented Sep 4, 2021

janosh commented Oct 1, 2021

dummdidumm commented Oct 1, 2021

janosh commented Oct 4, 2021

dummdidumm commented Oct 4, 2021 •

edited

Loading

janosh commented Oct 4, 2021

TrySound Oct 13, 2021

dummdidumm Oct 13, 2021

TrySound Oct 13, 2021

dummdidumm Oct 14, 2021

TrySound Oct 14, 2021

kaisermann Oct 14, 2021

kaisermann Oct 14, 2021

Better Typescript transpilation #392

Better Typescript transpilation #392

Conversation

dummdidumm commented Aug 22, 2021 • edited Loading

Before submitting the PR, please make sure you do the following

Tests

dummdidumm commented Aug 22, 2021 • edited Loading

kaisermann commented Aug 23, 2021

dummdidumm commented Aug 23, 2021

SomaticIT commented Aug 23, 2021 • edited Loading

dummdidumm commented Aug 23, 2021

SomaticIT commented Aug 23, 2021

dummdidumm commented Aug 23, 2021 • edited Loading

kaisermann commented Aug 23, 2021

dummdidumm commented Aug 23, 2021 • edited Loading

SomaticIT commented Aug 23, 2021

dummdidumm commented Aug 30, 2021

kaisermann commented Aug 30, 2021

SomaticIT commented Sep 1, 2021 • edited Loading

dummdidumm commented Sep 1, 2021

SomaticIT commented Sep 1, 2021

SomaticIT commented Sep 3, 2021 • edited Loading

Patch

Exemple:

Edit 1:

kaisermann left a comment

Choose a reason for hiding this comment

SomaticIT commented Sep 3, 2021

kaisermann commented Sep 4, 2021

janosh commented Oct 1, 2021

dummdidumm commented Oct 1, 2021

janosh commented Oct 4, 2021

dummdidumm commented Oct 4, 2021 • edited Loading

janosh commented Oct 4, 2021

TrySound Oct 13, 2021

Choose a reason for hiding this comment

dummdidumm Oct 13, 2021

Choose a reason for hiding this comment

TrySound Oct 13, 2021

Choose a reason for hiding this comment

dummdidumm Oct 14, 2021

Choose a reason for hiding this comment

TrySound Oct 14, 2021

Choose a reason for hiding this comment

kaisermann Oct 14, 2021

Choose a reason for hiding this comment

kaisermann Oct 14, 2021

Choose a reason for hiding this comment

dummdidumm commented Aug 22, 2021 •

edited

Loading

dummdidumm commented Aug 22, 2021 •

edited

Loading

SomaticIT commented Aug 23, 2021 •

edited

Loading

dummdidumm commented Aug 23, 2021 •

edited

Loading

dummdidumm commented Aug 23, 2021 •

edited

Loading

SomaticIT commented Sep 1, 2021 •

edited

Loading

SomaticIT commented Sep 3, 2021 •

edited

Loading

dummdidumm commented Oct 4, 2021 •

edited

Loading