Add compilation variables report option to allow getting all variables (even undeclared or internal)

[SomaticIT]

Description

This PR adds a new option varsReport to CompileOptions to allow getting the full variables report from the compilation process.

This allows external modules (like svelte-preprocess) to make their own decision about how to handle variables.

To avoid breaking change, the old behavior is still the default option (strict).

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

A first feature was implemented in the following PR:
#6169

References

Suggestion from @dummdidumm:
#6169 (comment)

TODO:

• Add new option to valid options list
• Adjust add_var to allow non-looked-up variable
• Adjust add_reference to add used variables
• Add a test in test/vars
• Adjust docs

Review

Before adding tests and docs about this new behavior I would like to collect your feedbacks about:

• Do you agree with the varsReport name?
• Do you agree with the string mode (full | strict | false)?
• Did the implementation respects your standards?

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

Many thanks

[dummdidumm]

Thanks for taking a stab at this! This isn't enough yet though

• new option needs to be added to valid options list
• you need to adjust add_var like this:

	add_var(variable: Var, add_to_lookup = true) {
		this.vars.push(variable);
		if (add_to_lookup) {
			this.var_lookup.set(variable.name, variable);
		}
	}

• you need to adjust add_reference like this:

// ...
 else {
			if (this.compile_options.varsReport === 'full') {
				this.add_var({name, referenced: true}, false);
			}
			this.used_names.add(name);
		}

• add a test in tests/vars
• adjust docs

[SomaticIT]

Thank you! 👍

Yeah I know, I wanted to collect your feedbacks before going further.
I was also looking to initiate a discussion in an appropriate place.

Your feedbacks looks good for me but I have some questions:

---

new option needs to be added to valid options list

I don't understand this one. Can you detail you thought?

Update: I think I understood, do you mean this?

svelte/src/compiler/compile/index.ts

Line 11 in dafbdc2

const valid_options = [

---

you need to adjust add_reference like this

Shouldn't we specify that this variable is undeclared in some way?

[dummdidumm]

Your feedbacks looks good for me but I have some questions:

new option needs to be added to valid options list

I don't understand this one. Can you detail you thought?

Update: I think I understood, do you mean this?

svelte/src/compiler/compile/index.ts

Line 11 in dafbdc2

const valid_options = [

Yes

you need to adjust add_reference like this

Shouldn't we specify that this variable is undeclared in some way?

That can be infered from the other properties (neither global nor injected is true, only referenced is). And since we don't care about this in the "return all variables" case anyway I thought it wouldn't make much sense to introduce another boolean.

[SomaticIT]

Should we also add a validation in validate_options function?

[SomaticIT]

I implemented your changes in add_var, add_reference and valid_options.

I also added a test in tests/vars but I'm not sure if it sufficiently coverage this feature.
Do you have suggestions for testing?

[dummdidumm]

I would add two more tests: One without a script block entirely (just {foo}), one with a script tag which contains a variable and in the template there's another one which is not declared, like

<script>let foo = ''</script>
{foo} {bar}

You also need to enhance the docs and mention the new option.

Other than that this looks good to me. @Conduitry does this look good for you, too?

[SomaticIT]

Missing tests added.
I will work on docs.

[SomaticIT]

The documentation was updated with the new varsReport option.
Does it seems understandable (and well-written)?

On my side, I'm all done.

Feel free to start the code review and give me your feedbacks.

[SomaticIT]

@dummdidumm, Thanks for this approval.

I tried to use this new option on svelte-preprocess to resolve sveltejs/svelte-preprocess#318.
The good news is that it seems to be less complex and to reduce maintenance.

However, if the template uses bind: directives, it will throw with ValidationError: nested is not declared.

Is there an option, to force the compilation even when errors occurs?
If not, should we implement it in this PR? Or another one?

Update:
After a quick investigation, it seems there is no way to avoid the compilation to throw when errors occurs.

Update 2:
I tried to implement a simple errorMode: "warn" option but it can has more impacts than this simple PR so I suggest to create another PR.

Indeed, the compiler expects component.error() to throw so it stops further processing. In some cases it can lead to uncaught errors. I think we will need to replace all component.error() occurences with return component.error() to mitigate the impact.

Update 3:
I updated my implementation of the errorMode: "warn" option by enforcing that all component.error() calls stop the flow (using return).

This seems to work well on svelte-preprocess side. I think we will need to make more tests on large codebases to see if there are no other impacts.

If you agree, I will open a third PR for this to have create a new discussion thread.

[dummdidumm]

This sounds good, maybe #4818 is handled by this as well.

[SomaticIT]

Thank you for merging this PR, I will restart my work on the svelte-preprocess. I have an idea: maybe we could enable the "double" compilation only when svelte-preprocess is running in dev mode.

[Conduitry]

This has been released in 3.39.0 - thanks!