Refactored preprocess for readability

[ehrencrona]

I found the code in the preprocess package a bit tangled. There was mutation happening in unexpected places and it was hard to see the data flow and dependencies. The index.ts was 300 lines which felt like too much. I found some of the variable and function names confusing.

I renamed a lot of stuff and restructured many of the functions. The process function now has a fairly clear structure and the mutations are all in PreprocessResult. I broke out everything related to search-and-replace with sourcemaps into replace_in_code.ts to reduce the length of index.ts to around 200 lines.

The diff is hard to read; it's probably better to just jump into the preprocess package and read it.

There should be no changes in behavior from these changes.

(the original impetus here was sveltejs/kit#19 because I started looking at if it was possible to isolate the async code and replace it with sync code whenever the preprocessor supported it; that should be slightly easier now)

Tests

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

[dummdidumm]

We should maybe wait on #5754 to get merged before doing this

[ehrencrona]

I hadn't seen #5754 but the merge shouldn't be too bad; it doesn't change all that much in preprocess/index.ts.

[ehrencrona]

Rebased!

[tanhauhau]

nit: i know there's nothing wrong, but it feels weird to spread property of an instance into an object.

const string_with_source_map = await replace_in_code(tag_regex, process_single_tag, source);
return { string: string_with_source_map.string, map: string_with_source_map.map, dependencies };

[ehrencrona]

I'd don't quite see it, but sure :)

[tanhauhau]

since process_tag already taken in result, why not calling result.update_source within?

[ehrencrona]

I feel this makes clearer what is going on: process_tag and and process_markup are (more or less) pure functions and you can clearly see in the calling code that the purpose is to update result. The fact that mutations were spread out through the code was actually the major issue I wanted to fix with this change.

(Note that the third parameter required by process_tag is actually not a PreprocessResult but a Source, which is a subset of it)

[tanhauhau]

hmm... didnt see you define this.source in PreprocessResult

[ehrencrona]

it's done by the public keyword in the constructor: constructor(public source: string, public filename: string)

[tanhauhau]

this feel like another PreprocessResult, except the location get offset.

{source: content, get_location: offset => source.get_location(offset + tag_offset), filename, file_basename})

would it be better if PreprocessResult have a method that can clone itself with an offset ?

[ehrencrona]

That makes sense. I gave it a stab; I am not entirely happy with the outcome, but it does feel like a step in the right direction so I'll leave it in.

I suspect more can be done but I'm a bit afraid of doing any change that is not guaranteed to do 100% the same thing as the existing code as I still can't quite overview what it does...

[tanhauhau]

👍🏻 for helping making it more readable, i left some comments where i find it still a bit hard to read after the refactoring.

[ehrencrona]

Thanks for the detailed feedback! I've applied the suggested changes now.

[halfnelson]

It certainly needed a refactor, nice job!
A previous but postponed refactor lives here: https://github.com/halfnelson/svelte/blob/1217c5b24a3499db2ed302436050a82a4419b2f6/src/compiler/preprocess/index.ts#L199
I mention this only because one of its features was to process script and markup tags in parallel which might be good to add to this PR

[benmccann]

@ehrencrona I just merged some additional preprocessor sourcemap support in #5854. If you're able to rebase this PR then hopefully we can merge this one as well

[ehrencrona]

Phew, that was a slightly tricky merge, but now it's rebased.

[benmccann]

thanks for going through the rebases!