add preprocess feature

[NicoCevallos]

I started copying the work done by @Conduitry in his/her fork and did some tests to fix it.
I created a package to use in the preprocessing returning the AST info for TypeScript and also a test project to show an example of how to use it.

[NicoCevallos]

I saw the CI for node 8 also fails for master branch because into devDependency the version of the package eslint is >=6.0.0, so today npm will install the version 7.2.0 which contains the code } catch { in the file eslint/lib/cli-engine/cli-engine.js:406. Also this version of eslint says into the engines section, that the supported node versions are ^10.12.0 || >=12.0.0, so this version is not correct for Node 8.
I did a little test and changing the eslint dependency version to ^6.0.0 in devDependencies, npm will install the version 6.8.0 that supports node version ^8.10.0 || ^10.13.0 || >=11.10.1 but I guess that is not the ideal solution, and as Node 8 is no longer supported since December 2019, the best solution would be to remove the tests for Node, and should include Node 14, so I just updated the PR.

[benmccann]

There's some discussion of the Node version in #65 as well

[NicoCevallos]

Thanks @benmccann for your comment. I guess both PR wont have conflict, but as you say, would be good to receive comments from other contributors in this package

[sarahscott]

What's the status on this?

[NicoCevallos]

@sarahscott I guess it'll be deprecated in favor of Svelte for VS Code extension + svelte-check package, please check this
https://svelte.dev/blog/svelte-and-typescript
Anyways I'll ask again about this, for non VS Code users

[victordidenko]

As far as I understand, svelte-check checks Typescript compiler's errors, it doesn't support eslint rules. So, adding Typescript support in this package is vital IMO.

[sdwvit]

i've tested this branch and this doesn't work yet unfortunately.

code to test
*.svelte file:

<script lang="typescript">
   let a: string = '';
</script>

error ParseError : Unexpected token

used sample config from preprocess package readme

[NicoCevallos]

@sdwvit I just updated the testing repo (https://github.com/NicoCevallos/eslint-svelte3-preprocess-test) and also I created a template repo with the latest version of svelte and related packages from the svelte template, you can check it here: https://github.com/NicoCevallos/svelte-template
Please copy with degit or clone any of those repos and test there what you did, as I could see there's no problem in the current version, but if you still have problems, please share a repo and let me know the node version you are running.

[d2c-sean-li]

@NicoCevallos I tested your patches in my own project. works for me at the moment.

[arschmitz]

@NicoCevallos have been using this in a new project for a few weeks and working also. Only thing we noticed is that the --fix option does not play nice with reactive syntax using $ and will mangle things a bit.

[lennyburdette]

Just wanted to weigh in that this patch works for me as well. Would love to see this merged.

[Rohit-L]

Is this project still maintained?

[jerriclynsjohn]

@sdwvit I just updated the testing repo (https://github.com/NicoCevallos/eslint-svelte3-preprocess-test) and also I created a template repo with the latest version of svelte and related packages from the svelte template, you can check it here: https://github.com/NicoCevallos/svelte-template
Please copy with degit or clone any of those repos and test there what you did, as I could see there's no problem in the current version, but if you still have problems, please share a repo and let me know the node version you are running.

I installed your svelte-template, If I take any file and edit it, VSCode shows an alert box with Saving 'Component.svelte': Getting code actions from "ESLint", "Stylelint" (configure). This just doesn't stop loading, it just keeps on going.

[jerriclynsjohn]

@sdwvit I just updated the testing repo (https://github.com/NicoCevallos/eslint-svelte3-preprocess-test) and also I created a template repo with the latest version of svelte and related packages from the svelte template, you can check it here: https://github.com/NicoCevallos/svelte-template
Please copy with degit or clone any of those repos and test there what you did, as I could see there's no problem in the current version, but if you still have problems, please share a repo and let me know the node version you are running.

I installed your svelte-template, If I take any file and edit it, VSCode shows an alert box with Saving 'Component.svelte': Getting code actions from "ESLint", "Stylelint" (configure). This just doesn't stop loading, it just keeps on going.

I had to change this setting to false to make it work, otherwise the editor was just preventing from saving the file.

"editor.codeActionsOnSave": {
		"source.fixAll": false
},

[sxxov]

@jerriclynsjohn I'm experiencing the same issue.

I did some debugging and found it was a problem coming from a combination of deasync (dependency of deasync-promise), and node-ipc (used by the eslint vscode extension). There would be no problem running eslint from command line, but it would freeze the eslint server from vscode. This makes me sorta suspect it to be a windows thing, inter-process weirdness and all that?

Running with "eslint.debug": true in vscode's "settings.json", the trace would end on:

2020-12-11T16:23:42.099Z eslint:linter Linting code for C:\...\App.svelte (pass 1)
2020-12-11T16:23:42.099Z eslint:linter Verify
2020-12-11T16:23:42.099Z eslint:linter With ConfigArray: C:\...\App.svelte
2020-12-11T16:23:42.099Z eslint:linter Apply the processor: 'svelte3/svelte3'

I did some (painful) console.log-ging and found it would freeze upon any await call (even something like await null;), with the culprits coming straight from the svelte compiler (compiler.preprocess). I also did some poking around with the desync module, it seems like the event loop is running and pausing as it should, but promises aren't being resolved.

Not sure what a course of action would be though, something hacky like spawning a new process for the compiler, having it write to disk, pinging the disk synchronously on the current process?.

[sxxov]

@jerriclynsjohn I've got mine to work, and have a utensil to share for it too (https://github.com/Sxxov/eslint-svelte3-preprocess). It's an ugly solution that very much shouldn't be used in anything serious (using desync to poll worker_threads every n milliseconds), but it works for now.

Excited for how the final version of preprocessor support will look like.

[gustavopch]

@sxxov Even with your fork, sometimes VSCode still doesn't overlay errors/warnings in the editor for lang="ts" (I have eslint.validate includes svelte in my config). Removing that attribute and going back to plain JS makes the overlays appear again. I still have no clue why that happens.

EDIT: Upgrading ESLint from v6.8.0 to v7.15.0 seems to have fixed it.

EDIT 2: When running through the CLI, it will hang and not finish the process.

[sxxov]

@gustavopch Hmm, interesting. I haven't tested this much, but I did notice some version problems somewhere (it working when i popped the test build straight in but not after i installed the required node_modules).

Could it be that it's just slow? It takes around 3 seconds per lint task (vs ~30ms for pure typescript), and it runs on every keystroke. Normally you'd just add a sleep timer, or tolerance per key, but having everything single threaded makes that super difficult. This might be because it's building from scratch everytime instead of using caches (building svelte+typescript with snowpack is nearly instantaneous)

For the CLI problem, I'm not sure for that either, as that was my known good. Could it be that you're running the global version instead of the project version, which may be 6.x.x? You could run with --debug and see if it freezes at ...preprocessor: svelte3/svelte3

[gustavopch]

@sxxov At first, it seems to be slow, but that's just because there's an error here which makes the loop continue even after isDone === true: it should use && instead of ||. After fixing that line and adding some logs, I can see that the result comes pretty fast, but the editor is not overlayed with the errors/warnings when ESLint v6.8.0 is used. Upgrading to v7.15.0 and reloading the window immediately solves the problem. I really think the version is the important thing here.

Regarding the CLI: no, it doesn't hang on the preprocessor step. It completes all the linting and even prints the errors/warnings to the console. It just happens that the process doesn't exit after that. It keeps running. I tried to force worker.terminate() to no avail.

[sxxov]

@gustavopch I'm not sure where the bug is, as that code is meant to break the loop when either isDone === true or the timeout is finished.

The entire statement would evaluate to true if either one would succeed. The freeze would only theoretically happen if you'd used &&, which may cause the loop to become infinite when is done returns undefined (on timeouts or syntax errors sometimes).

(the formatting on that statement is pretty wack though)

The speed increase you're looking at might be the linter receiving undefined from the preprocessor and just ignoring it. The newer eslint version might just be linting javascript, while the older one outputting nothing.

The CLI part is interesting though. Maybe you could try with the original branch that i forked from and see if that would work? As I could only get the original branch to work on the CLI. worker.terminate() shouldn't cause the script to exit though, as it's not waiting for the worker to end (it reuses the same worker), but it simply polls for the results. I suspect the culprit might be deasync again though. I'm very much still foreign to this codebase, but it would seem like there's a lot more work to be done here xd

Edit: it just clicked for me, and you're right, && for life. Got confused since for loops need a false to break instead of a true!

[sxxov]

@gustavopch I think I've cracked the freezing! As well as improved performance ~10x (~300ms -> ~30ms per keystroke) according to some preliminary testing.

Achieved by completely getting rid of deasync, using sharedArrayBuffer to transfer data between the threads, and using Atomics.wait to pause the thread without pinning CPU.

Also, a lot of Developer: Reload Window key presses xd.

Check it out (https://github.com/Sxxov/eslint-svelte3-preprocess)

EDIT: Running eslint from the CLI still hangs after it finishes preprocessing the files, as the worker is still alive. Not sure how to fix this without jeopardizing performance (creating a new worker every time the function is called costs ~300ms, suspect it's a scoping thing?)

[gustavopch]

@sxxov Finally I've been able to try your last implementation. Thanks for sharing it with us!

I have some suggestions:

1. AFAICT you don't need to manually delete result.toString as it'll be automatically ignored by the structured clone algorithm.
2. You can move the declaration of the buffers, views and worker directly into the main function (but not into the inner anonymous function as you noted in the comment). It will help cleaning the code with fewer arguments being passed around and won't affect the performance.
3. You don't need that eslintSveltePreprocess variable. You can simply export the result of main() like module.exports = main(); export default module.exports.
4. This if block won't ever be reached due to the try-catch above.
5. I've removed those return statements from the preprocess callbacks and it continued to work just fine. I think they're not needed.
6. A workaround to prevent the CLI from hanging is to add clearTimeout(timeout); timeout = setTimeout(() => process.exit(0), 1000) to the end of the main function (of course, declare let timeout in the scope of main).

I updated my gist with your improvements: https://gist.github.com/gustavopch/134513fa7c1f30050e968b5570c26994 (I'm a heavy TS user, but I think TS is not adding much value there, that's why I'm using vanilla JS in my gist).

[sxxov]

@sxxov Finally I've been able to try your last implementation. Thanks for sharing it with us!

I have some suggestions:

• AFAICT you don't need to manually delete result.toString as it'll be automatically ignored by the structured clone algorithm.
• You can move the declaration of the buffers, views and worker directly into the main function (but not into the inner anonymous function as you noted in the comment). It will help cleaning the code with fewer arguments being passed around and won't affect the performance.
• You don't need that eslintSveltePreprocess variable. You can simply export the result of main() like module.exports = main(); export default module.exports.
• This if block won't ever be reached due to the try-catch above.
• I've removed those return statements from the preprocess callbacks and it continued to work just fine. I think they're not needed.
• A workaround to prevent the CLI from hanging is to add clearTimeout(timeout); timeout = setTimeout(() => process.exit(0), 1000) to the end of the main function (of course, declare let timeout in the scope of main).

@gustavopch Heyo thanks for the free code review! Regarding the suggestions:

1. My bad on that one xd. It's one of the things leftover from when I was still using postMessage instead of SharedArrayBuffer, since structured clone would throw if it found a function, unlike JSON.stringify which will just discard it
2. I think I remember trying that, and that resulted in the same performance drop. Not sure why since it wasn't recreating anything. It became one of the bigger reasons that made me suspect it was a closure thing instead of something dumb I did
3. I'm not too familiar with CommonJS so I didn't know you could just export that, thanks!
4. That if block is definitely left behind code from the postMessage implementation since errors mostly can't be structured cloned, my bad again!
5. I encountered errors in some versions of ESLint without those returns (I think <7), so I just added them for compat sakes since it didn't affect anything in >=7
6. Killing the main process would takedown the entire ESLint server if it was running inside of one. Killing after every task would return the performance hit as it would need to recreate the worker.

TS was mainly used because of the original repo xd. It helped a lot with prior implementations where I had to deal with postMessage and metadata that had to be sent to the worker. It also didn't really get in the way after that so I just kept it.

Thanks again tho! I'll probably have commits up by today fixing the things

[gustavopch]

@sxxov Thanks for the answers.

2. If you declare the worker inside the inner anonymous function, a new worker will be created for each file that's being linted (if ESLint is linting 10 files, you'll have 10 workers because the anonymous function is called once for each file). On the other hand, if you declare it inside the main function (but outside the inner anonymous function), a single worker will be created because main runs only once when the process starts; you'll have a single worker regardless of how many files are being linted (you can try using the implementation from my gist to test whether it has the same performance as yours).
5. Good to know, thanks.
6. Hmm, so perhaps prevent killing the process if it's running within a VSCode extension. Not sure, but I think we can check that with const isRunningWithinExtension = Boolean(process.env.VSCODE_PID). Maybe there's a better way. (Edit: probably better to check whether it's running within ESLint CLI as we also don't want to kill the process in other editors beyond VSCode, so perhaps const isRunningWithinCli = process.env.npm_lifecycle_event === 'eslint')

[sxxov]

@gustavopch

2. Thanks for the explanation, that should be right. I got your comment confused with the "outer" anon function that returns the "inner" anon function, which I've tried and that gave bad perf even though it doesn't recreate workers (just factory function). Can't remember if doing it in the main function caused any problems so there's some poking around to be done.

EDIT: Wait, I completely forgot I had to declare them outside so I could pass it into the worker instead, not because of performance at the end! I didn't wanna include the worker instantiation in main since I felt it wasn't really the job of what the main thread was doing, but I see why moving it inside should look clearer since it is actually done by main.

[gustavopch]

@sxxov Forgot to mention that import.meta.url was crashing here, probably because I'm using Node.js v12 which doesn't support ESM.

[sxxov]

@gustavopch Implemented most of your suggestions. The ones not implemented i have put some comments above them explaining why as well. Managed to figure out a way to get CLI's to not hang, while not needing to recreate the worker/rely on an actual timeout, too!

sxxov/eslint-svelte3-preprocess@1d221db

[gustavopch]

@sxxov Great! Now, you mentioned those return statements exist for compatibility with ESLint <7, but I can't get it to work with ESLint v6.8.0 (I've tested with v7.0.0 and v7.15.0 and it works just fine, exact same code, with or without return statements). I'm trying to make it work with ESLint v6.8.0.

[dummdidumm]

For those of you who participate in this PR because they want TypeScript support: With version 3.1.0, this plugin now supports TypeScript.

[NicoCevallos]

For those of you who participate in this PR because they want TypeScript support: With version 3.1.0, this plugin now supports TypeScript.

Hi @dummdidumm , thanks for the good news but it doesn't cover scenarios where the user uses a custom preprocessing for other script languages. The intention of the preprocess feature is to allow the user to customize their preprocess as is done in the build process.

[dummdidumm]

I know, it's a solution specific to the TypeScript problem, not the general preprocessor problem. That's why I have written "for those of you who ... want TypeScript support".

[NicoCevallos]

@sxxov Great! Now, you mentioned those return statements exist for compatibility with ESLint <7, but I can't get it to work with ESLint v6.8.0 (I've tested with v7.0.0 and v7.15.0 and it works just fine, exact same code, with or without return statements). I'm trying to make it work with ESLint v6.8.0.

@sxxov & @gustavopch Thank you so much for your ideas to deal with the VS Code integration. I didn't know anything about workers and found your work very useful to deal with it.
I just published a new version of the eslint-svelte3-preprocess
https://github.com/NicoCevallos/eslint-svelte3-preprocess/releases/tag/v0.0.5

[NicoCevallos]

I know, it's a solution specific to the TypeScript problem, not the general preprocessor problem. That's why I have written "for those of you who ... want TypeScript support".

Understood, but... why not to move forward with this feature which allows to use the same configuration for this plugin, build process and IDE extensions like Svelte for VSCode?

[dummdidumm]

Understood, but... why not to move forward with this feature which allows to use the same configuration for this plugin, build process and IDE extensions like Svelte for VSCode?

We chose to add support for specifically TypeScript first because it was easier to add support for and many people want it (more than those who want general preprocessor support I assume). The focus on a specific implementation also made for a nicer API and there's no need to use packages like deasync. This doesn't mean that there will never be general preprocessor support in this plugin.

[sxxov]

there's no need to use packages like deasync

deasync can already be removed by using Atomics.wait on the main thread and running async tasks in worker threads. A perfect solution? No, but imo good & performant enough.

With that said, I do see where you're coming from, since the scope of stable preprocessing is really big. I personally have encountered many nasty bugs while poking around (looking at you, prettier). Hope this feature makes it into the plugin one day!

[Monkatraz]

Considering the mild wackiness (it's not too bad now) of this async system, is it possible to just ship out-of-the-box support for everything that svelte-preprocess natively supports and that can be done sync? If a preprocessor is unhandled, the Atomics.wait solution could be used, but if it is handled it could use a built-in synchronous solution that is probably a lot faster (less latency, maybe not performant).

To be clear - I really want this merged, I use Stylus which is async. It's just that worker creation overhead adds some noticeable latency (last I checked), and it might be worth it to minimize that if possible.

[NicoCevallos]

Understood, but... why not to move forward with this feature which allows to use the same configuration for this plugin, build process and IDE extensions like Svelte for VSCode?

We chose to add support for specifically TypeScript first because it was easier to add support for and many people want it (more than those who want general preprocessor support I assume). The focus on a specific implementation also made for a nicer API and there's no need to use packages like deasync. This doesn't mean that there will never be general preprocessor support in this plugin.

@dummdidumm I don't see any message saying "hey, can we find a solution without using deasync?".
If nobody gives any feedback how the people will want to collaborate?

[aewing]

@dummdidumm I don't see any message saying "hey, can we find a solution without using deasync?".
If nobody gives any feedback how the people will want to collaborate?

@NicoCevallos I think the preference to avoid deasync was established here: #10 (comment)

[sxxov]

To be clear - I really want this merged, I use Stylus which is async. It's just that worker creation overhead adds some noticeable latency (last I checked), and it might be worth it to minimize that if possible.

@Monkatraz If you haven't checked my previous comments, I've got some good news xd. I seemed to have cracked that with ~10ms of overhead on small files running on workers, instead of ~300ms to instantiate them everytime.

Further benchmarking is required but I think this solution will be able to scale to medium/large sized files without an issue (been using it personally on files with hundreds of lines, don't ask why). The only bottlenecks I'm aware of atm is the 50mb fixed alloc for SharedArrayBuffer and the JSON encoding/decoding.

Your solution of using ""native"" sync solutions is interesting but I'm not sure what that would look like, any pointers?

[Monkatraz]

Your solution of using ""native"" sync solutions is interesting but I'm not sure what that would look like, any pointers?

It would just be like the current TypeScript support, except with any other preprocessor.

Although, with a 10ms instantiation latency, I seriously doubt it's worth doing sync.

[NicoCevallos]

@dummdidumm I don't see any message saying "hey, can we find a solution without using deasync?".
If nobody gives any feedback how the people will want to collaborate?

@NicoCevallos I think the preference to avoid deasync was established here: #10 (comment)

@aewing I didn't relate the "deasync" word with the deasync package... but in this PR, I don't see anything except when @sxxov who started to provide a solution for that.
In fact I'm remembering (lol) I didn't use deasync here, I used it on another package which provides the preprocessing info, allowing anybody to provide another solution, so deasync is not a problem here.
Anyways, when I started with deasync that worked as extension, and worked for CLI, then with a VSCode update it didn't work as extension, but still worked for CLI.
But talking about today, it's working pretty well, the preprocessor I create is now working and solving all the things mentioned by @Conduitry in the comment you mentioned, applying source map to show the error in the correct place, and allowing ESLint fixes