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 https://github.com/sveltejs/eslint-plugin-svelte3/pull/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!

[gustavopch]

@Sxxov I tried to implement it myself from scratch following what I learned from your implementation and it seems to work 100%: https://gist.github.com/gustavopch/134513fa7c1f30050e968b5570c26994

I think the culprit for the process hanging in your implementation was this listener which was never removed (so the worker thread would hang even after ESLint was done).

[sxxov]

@gustavopch Wow your implementation is so much more simpler haha.

This difference might have something to do with the eslint CLI running on one node instance, while the vscode extension that I tested with would spin up a separate "server" instance that it reuses?

I had assumed creating a new worker on every keystroke would've been bad but haven't tested it, seems to work okay though from what you've said. You could probably use workerData to save on the initial postMessage call as well.

Thanks for the help!

[jerriclynsjohn]

as never removed (so the worker

Tried this and at every <style lang="postcss"> tag I get a red squiggly which says Semicolon or block is expected eslint(ParseError) . This is also appearing inside the style tag.

[gustavopch]

@jerriclynsjohn Are you using my Gist? If so, I think you'll need to pass the postcss config to sveltePreprocess().

[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.