fix lint warnings

[antony]

To be merged after sveltejs/eslint-config#4 is merged and the config tagged.

This leaves only one lint warning, which is a bit of a complex fix and will need to be debated - the fact that we mutate the node assert builtin, in tests for Svelte. I've mostly fixed this locally, but there are still test failures, so this covers the low hanging fruit and cleans up all the other rules, with a view to fixing that one later.

• This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
• Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Todo:

• Get svelte-eslint merged and tagged
• Update dependency in package.json to point to new tag

[Conduitry]

I vote we use a comment to disable the last warning for now (// eslint-disable-line no-import-assign). The ESM in the file may be invalid, but the CommonJS that we know it's getting compiled to is fine. We can revisit a cleaner solution to this later, but I think it would be good to officially get rid of all warnings at once.

[antony]

I vote we use a comment to disable the last warning for now (// eslint-disable-line no-import-assign). The ESM in the file may be invalid, but the CommonJS that we know it's getting compiled to is fine. We can revisit a cleaner solution to this later, but I think it would be good to officially get rid of all warnings at once.

I had this exact conversation with @benmccann - It's up to you, but my personal preference is to leave it there. Mutating a node built-in isn't great at all, and is the exact sort of thing linting exists to prevent. If we mute it, it'll become forgotten and just be another exclusion to the linting rules. It's something I'd probably want to be constantly (gently) reminded of until it's fixed.

[benmccann]

I'd be fine with just disabling the warning. I took a look at fixing it and I'm not sure it'd be very high ROI. There's nearly 700 places the function is used. It didn't look like VS Code's refactoring would work on it either - though I'm new to VS Code, so maybe don't have my project setup correctly or something. But without an automated way of changing it, I don't think it's worth our time. And if we're not realistically going to fix it, then the warning just becomes an annoyance

[antony]

I'll concede that it's probably more effort to fix than it's worth, and at least it will hopefully prevent further issues.

I'll just ignore it for now and update this PR.