Pass full markup source to preprocessors

[SomaticIT]

Hi,

This is my first PR on this project so I want to thank you for building this amazing framework/compiler/library(?) 👍 !

Why?

This PR was done to allow the typescript preprocessor to better detect used imports in a Svelte component and automatically strips type/value mixed imports.

It will help resolving this issue: sveltejs/svelte-preprocess#318.
You can see this comments for a road to implementation:

• TypeScript: Better preprocessing to make importsNotUsedAsValues unnecessary svelte-preprocess#318 (comment)
• TypeScript: Better preprocessing to make importsNotUsedAsValues unnecessary svelte-preprocess#318 (comment)

To summarize, it will alllow this code:

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

<p>...</p>

to become:

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

<p>...</p>

Please note that this PR is not resolving the issue by itself but it is needed to allow this issue to be resolved in svelte-preprocess.

Additional notes

Testing

I did not add a test because I'm not sure it is necessary. It does not break existing tests and the only test I see would be to check that we have the source param available in the preprocess function. (it seems pretty useless)

Do you want me to add this test?

Question

I asked this question in the svelte-preprocess issue referenced before:

Is there a safe way (no regexp) to extract JS expressions (or variables) from markup without doing a full svelte compilation?

Update

I see that the windows+node v8 test failed because of a timout issue. I think that it's only an outage on Github. Is it possible to rerun this job to ensure that everything is OK?

Since all others tests are successful, I think there is no problem, but I like green ticks :-)

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

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

[kaisermann]

Is there a safe way (no regexp) to extract JS expressions (or variables) from markup without doing a full svelte compilation?

I'm not sure if we would be able to use the svelte parser with generate: false for this (since script and styles wouldn't have been preprocessed yet), but I'd start from there.

[SomaticIT]

I'm not sure if we would be able to use the svelte parser with generate: false for this (since script and styles wouldn't have been preprocessed yet), but I'd start from there.

Thank you for this response.

I'm working on an experiment where I remove the script and style tag then run svelte.parse to extract the AST. 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?

[dummdidumm]

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

[SomaticIT]

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]

@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?

[SomaticIT]

Wow, checks have failed in cascade. I'm not sure why...
I suspect an external reason because this change should not affect existing codebase.

Can we rerun them?

[SomaticIT]

Any news?

This PR is needed to implement sveltejs/svelte-preprocess#318.

Thank you

[dummdidumm]

I added a code comment above which you didn't answer yet.

[SomaticIT]

Ooops! I'm sorry, I didn't see it 😅!
I answered directly in your code review.

[benmccann]

I wouldn't understand exactly what markup contains from just the name. My first guesses would be the html or the entire template contents or something else. Can we document it?

[SomaticIT]

@benmccann, you're totally right.

@dummdidumm, thanks for writing this.

Next steps are on the svelte-preprocess side.

[dummdidumm]

I think we can add one more PR to Svelte to make vars return all variables in the template, even if they are not defined elsewhere (currently they are filtered out). That removes the need for any custom AST walk / transformation of code.

[SomaticIT]

That sounds great!

If we want to do this, I think we need to add a new option on the svelte compile function to avoid changing existing behavior (which is safer).

Maybe something like:

svelte.compile(markup, { generate: false, includeAllVariables: true });

or:

svelte.compile(markup, { generate: false, variables: "all" });
// variables could be: "all" or "strict"

[SomaticIT]

There is also one other solution that could work for us.
If we could export (in some way) the Component file from the svelte compiler, we would be able to do this without changing the svelte compile behavior:

const ast = parse(source, options);
const component = new Component(ast, source, "TempComponent", options); // make stats and warnings optionals.

console.log(component.vars); // all vars are available here

What do you think?

[dummdidumm]

This is not needed, if you pass generate: false, the render step is skipped, so performance-wise there's no difference then.

[Conduitry]

This has been released in 3.38.0.