WIP: Improve TypeScript imports preprocessing (#318) #342

SomaticIT · 2021-04-14T12:29:13Z

/!\ This is a work-in-progress. DO NOT MERGE!

Introduction

I open this PR to init the work and the collaboration on a resolution for #318.

Note: This implementation requires an unreleased version of svelte to work.
Note 2: You will find below a way to setup a test environment to test this implementation on your projects.

Background

Thanks to @dummdidumm advices, I added 3 features on svelte, some have been merged, some are waiting for review:

Pass full markup source to preprocessors svelte#6169
Add compilation variables report option to allow getting all variables (even undeclared or internal) svelte#6192
[feat] Add errorMode option to compile to allow continuing on errors (and mark them as warnings) svelte#6194

These features enable 2 required things:

It makes svelte pass the full source markup to preprocessors
It allows to use svelte.compile to get a full report of used variables in markup

Description

This PR makes two improvements.

First, it takes the markup passed by svelte and make it available to transformers.
This markup is optional here to be backward compatible with previous versions of svelte.

Then, it uses the full markup, strips the script and style tags and pass it to svelte.compile to get a report of all used variables in the template. This list of variables is appended to the TypeScript code to allow the TypeScript compiler to correctly handle imports. Thanks to this, we can remove the custom importTransformer when markup is available.

TODO

This is a work in progress.
First of all, it needs above PRs to be merged and released on svelte side.

Then, it misses some cases:

Handling context="module" scripts
Stripping the injected code from sourcemaps
Adding more tests
Testing on real-world project

For this last task, I will need the help of the community 😉.

I also have very few experience on sourcemap manipulation so if someone can help on this...

Setup test environment

If you want to test working on this PR, you will need to do the following:

# Clone the fork with all svelte PRs merged
git clone https://github.com/SomaticIT/svelte.git
cd svelte

# Switch to the appropriate branch
git checkout @release/custom

# Prepare the project
npm install
npm run build

# Link the project globally
npm link

# Clone the fork of svelte-preprocess
cd ..
git clone https://github.com/SomaticIT/svelte-preprocess.git
cd svelte-preprocess

# Switch to the appropriate branch
git checkout @feature/typescript-imports

# Link svelte locally
npm link svelte

If you want to test this feature in your project (I will need your feedbacks):

# Do the previous steps then from the svelte-preprocess dir
npm run build
npm link

# In your project dir
npm link svelte
npm link svelte-preprocess

Performances impact

On my computer, I benchmarked the preprocessing of this svelte file: test/fixtures/TypeScriptImports.svelte.
Before this PR: ~30ms
After this PR: ~55ms

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

dummdidumm · 2021-04-14T12:37:18Z

src/transformers/typescript.ts

+}): string {
+  if (!markup) return content;
+
+  const { vars } = compile(stripTags(markup), {


Since the Svelte compiler will throw errors on unknown properties, this needs to be surrounded with a try-catch for backwards-compatibility.

If I'm correct, it will depend on the release of svelte. If the 3 PRs are merged and released in the same time, there will be no problem since the markup will not be passed in previous versions. Thus, it will not try to compile it.

Am I correct?

Ah right, yes that is correct. Should be documented in the code then just to be sure for future readers.

Maybe we should inform svelte maintainers that the 3 PRs should be released together.
Do you agree?

If it's not possible, using a try-catch will solve the issue

dummdidumm · 2021-04-14T12:46:23Z

Thanks for taking a stab at this!
Regarding script context="module": In that case the whole instance script needs to be appended, too. To keep it consistent I suggest to separate the generated code with some comment like // __svelte-preprocess__ start generated code and split on that.
Regarding source maps: I don't know much about them either, I only know that mapping lines are separated by ; , which could be used to determine where to cut off the mappings. If sourcesContent is returned, we can override that with the original code. I'm not sure what else we need to adjust.

SomaticIT · 2021-04-14T22:48:52Z

Thank you for your feedbacks.

To keep it consistent I suggest to separate the generated code with some comment like // svelte-preprocess start generated code and split on that.

I was considering using something like const $$$$$$$$$ = null; to be sure that typescript will not remove it.
I also thought that this could allow us to retrieve it from the source map but I'm not sure that's the right way.

Regarding script context="module": In that case the whole instance script needs to be appended, too.

I agree, I will add this with an appropriate test.

Regarding source maps:...

I need to make some research on this..

Do you know a svelte maintainer who could help?

- 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

dummdidumm · 2021-04-15T06:36:15Z

@milahu could know more

SomaticIT · 2021-04-15T10:19:49Z

Hi @dummdidumm,

I added a test for context="module" script tags.
This seems to work well.

I noticed that we use common functions with modules/markup.ts so I also did a little refactoring to reuse the logic.

- 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

SomaticIT · 2021-04-15T21:25:24Z

After some studies and tests, I managed to handle the source map transformation.

Process

Here is a little summary of the process:

Inject variables and produce a source map
Transpile typescript and produce a source map
Strip variables and produce a source map
Concatenate the source map chain

Note: For backward compatibility, this behavior is disabled if no markup is passed to the transformer.

Note 2: This process is only applied if the sourceMap option is true. Else, it does not bother with sourcemaps and keep the previous behavior.

Refactoring

This implementation introduced a lot of complexity in the transformer.
So I splitted the code in the transformer to make it more readable.

Drawbacks

This implementation introduces two new dependencies:

magic-string allows to manipulate string with source map support.
sorcery allows to concatenate a source map chain.

Fun fact: these libraries are written by Rich Harris.

Can we reuse these dependencies?

magic-string could be reused to handle source map in the replace transformer.

Maybe sorcery could be reused to concatenate source maps when multiple transformers are used during autoProcess (eg: replace + typescript + babel).

Additional notes

I was not sure of a good way to test this source map transformation.

I ended writting a simple test to check that source map is produced and I used the great source-map-visualization tool (link).

You can find a screenshot of the result below.

Questions

Do you have any idea to better test this?

Also, do you agree with the refactoring?

Screenshot

dummdidumm · 2021-07-20T09:26:01Z

Now that the needed PRs are merged (new Svelte release pending), we can continue working on this. Some thoughts

Since the "pass markup to preprocessors" PR was released already, this means we need the try-catch-approach to catch the "Svelte version does not support these options yet" case. That try-catch could be at the very top of the transformer, so if anything breaks during the new approach we fall back to the old approach
I think (didn't test) it's possible to have an import inside the instance script that is only used within the module script block. If that's possible, then we need to append the module script to the instance script, too. I think appending is fine - this will result in invalid code but in "transpile only" mode TypeScript will just go through the stuff one-by-one

I'm on vacation the next 3 weeks so I can't look into this in the near future.

dummdidumm · 2021-08-17T07:22:23Z

@SomaticIT I'll try to pick this up. The proposed solution is very smart, from what I see it only needs some minor adjustments.

Since the three PRs in Svelte core were not merged as one, we need that try-catch solution
We either need to add some logic to only return the transpiled instance/module script content, or we could make this a markup transformer instead. The latter would break the order, but would make the code probably a little easier and faster because we don't need to do the work twice. @kaisermann thoughts?

SomaticIT · 2021-08-17T09:02:41Z

@dummdidumm, thank you.

I'm currently on vacation. I will be back next week. I would be happy to help.

I agree with the try-catch solution.
I'm not sure I understand your second thought, if I remember correctly, only the transpiled instance/module script is already returned.

I also had an idea that could lead to better performance : maybe we could make svelte pass the dev option to preprocessor. This special transpilation process is only needed in dev mode since rollup drops unused artifacts. It will also avoid having a try-catch.

What do you think?

dummdidumm · 2021-08-22T09:18:51Z

Ah yes you're right, the injected code is always below the real code, and it's stripped away. So this should work then.

I tested this out on a project and ran into one issue: When there's no code after transpilation, because the code is type-defs only, the mappings are an empty string, which breaks sorcery (also see Rich-Harris/sorcery#167). A check is needed to return early in this case. Other than that, this works great 🎉

Another thing is: When using the TypeScript transpiler, it's now necessary to also install source-map and sorcery, which is somewhat of a breaking change. @kaisermann what's the best strategy here? Should we add these libraries as dependencies, or bundle them, or request the user to install them themselves? Edit: A non-breaking strategy would be to make the new way to transpile opt-in, with default off, and dynamically load source-map and sorcery, too. The only drawback would be to make people more aware of this (for SvelteKit, we could make the opt-in part of the starter template). However, if TypeScript implements support for strict separation of types and values (PR pending, maybe it's part of 4.5), these changes right here might not even be needed anymore for people using the newest TS version and the new default would be for users to use that new TypeScript setting. (btw this doesn't mean I'm not in favor of this PR anymore, I still am).

I also had an idea that could lead to better performance : maybe we could make svelte pass the dev option to preprocessor. This special transpilation process is only needed in dev mode since rollup drops unused artifacts. It will also avoid having a try-catch.

I'm not sure if we can do this, this relies on the people having some kind of tree shaking as part of their build, which is not always the case. If we make this new behavior opt-in, people could manage that and say "on production builds I know this will work because of treeshaking" and turn if off.

dummdidumm · 2021-08-22T09:28:40Z

src/transformers/typescript.ts

+    filename,
+    sourceMapChain,
+  });
+


This fixes the sorcery error that happens on empty code/maps. A test case should be added to handle this.

Suggested change

if (!code) {

return { code, diagnostics };

}

kaisermann · 2021-08-23T11:59:51Z

I've been a bit AFK these last two months, so I'll need to get my bearings regarding this whole scenario again.

it's now necessary to also install source-map and sorcery, which is somewhat of a breaking change. @kaisermann what's the best strategy here?

We are already "educating" people to install what they're going to need for their respective workflows. However, I'm also all up for simplicity and, as it seems, most people are using/will use Svelte with Typescript at some point. I'd like to keep only the "main tools" as "peer dependencies" (ts, postcss, stylus, sass, etc) if possible, although postcss-load-config kind of already goes against this 🤔 .

What's keeping us from having the advanced type import as the default behaviour? Just these dependencies or does it behaves differently in other ways? Is there an advantage for having both the current and new behaviour?

However, if TypeScript implements support for strict separation of types and values (PR pending, maybe it's part of 4.5)

Can you link the PR here?

dummdidumm · 2021-09-04T22:03:18Z

@kaisermann this can be closed now

feat: pass source markup to transformers (if provided)

12c1157

dummdidumm reviewed Apr 14, 2021

View reviewed changes

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

SomaticIT mentioned this pull request Apr 15, 2021

TypeScript: Better preprocessing to make importsNotUsedAsValues unnecessary #318

Closed

SomaticIT force-pushed the @feature/typescript-imports branch 2 times, most recently from cfa861e to e5a73db Compare April 15, 2021 10:18

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

SomaticIT force-pushed the @feature/typescript-imports branch from 435a6d5 to 8c97341 Compare April 15, 2021 21:19

milahu mentioned this pull request Apr 15, 2021

request: add { importsNotUsedAsValues: preserve-values } to compilerOptions microsoft/TypeScript#43687

Closed

5 tasks

dummdidumm reviewed Aug 22, 2021

View reviewed changes

kaisermann mentioned this pull request Aug 23, 2021

Better Typescript transpilation #392

Merged

4 tasks

kaisermann closed this Sep 4, 2021

SomaticIT deleted the @feature/typescript-imports branch September 8, 2021 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Improve TypeScript imports preprocessing (#318) #342

WIP: Improve TypeScript imports preprocessing (#318) #342

SomaticIT commented Apr 14, 2021 •

edited

Loading

dummdidumm Apr 14, 2021

SomaticIT Apr 14, 2021 •

edited

Loading

dummdidumm Apr 14, 2021

SomaticIT Apr 14, 2021

dummdidumm commented Apr 14, 2021

SomaticIT commented Apr 14, 2021

dummdidumm commented Apr 15, 2021

SomaticIT commented Apr 15, 2021

SomaticIT commented Apr 15, 2021 •

edited

Loading

dummdidumm commented Jul 20, 2021

dummdidumm commented Aug 17, 2021

SomaticIT commented Aug 17, 2021

dummdidumm commented Aug 22, 2021 •

edited

Loading

dummdidumm Aug 22, 2021

kaisermann commented Aug 23, 2021

dummdidumm commented Sep 4, 2021

+if (!code) {
+  return { code, diagnostics };
+}

WIP: Improve TypeScript imports preprocessing (#318) #342

WIP: Improve TypeScript imports preprocessing (#318) #342

Conversation

SomaticIT commented Apr 14, 2021 • edited Loading

Introduction

Background

Description

TODO

Setup test environment

Performances impact

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

Tests

dummdidumm Apr 14, 2021

Choose a reason for hiding this comment

SomaticIT Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

dummdidumm Apr 14, 2021

Choose a reason for hiding this comment

SomaticIT Apr 14, 2021

Choose a reason for hiding this comment

dummdidumm commented Apr 14, 2021

SomaticIT commented Apr 14, 2021

dummdidumm commented Apr 15, 2021

SomaticIT commented Apr 15, 2021

SomaticIT commented Apr 15, 2021 • edited Loading

Process

Refactoring

Drawbacks

Can we reuse these dependencies?

Additional notes

Questions

Screenshot

dummdidumm commented Jul 20, 2021

dummdidumm commented Aug 17, 2021

SomaticIT commented Aug 17, 2021

dummdidumm commented Aug 22, 2021 • edited Loading

dummdidumm Aug 22, 2021

Choose a reason for hiding this comment

kaisermann commented Aug 23, 2021

dummdidumm commented Sep 4, 2021

SomaticIT commented Apr 14, 2021 •

edited

Loading

SomaticIT Apr 14, 2021 •

edited

Loading

SomaticIT commented Apr 15, 2021 •

edited

Loading

dummdidumm commented Aug 22, 2021 •

edited

Loading