Skip to content

chore: validate promise handling #11842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 4, 2024
Merged

chore: validate promise handling #11842

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
843573a
chore: don't swallow rejected promise errors
benmccann May 30, 2024
73eff42
merge main
benmccann Jun 1, 2024
0611bef
ignore rules rather than adding catch blocks. add async to functions …
benmccann Jun 1, 2024
474f3d7
remove redundant asyncs
Rich-Harris Jun 3, 2024
3f2dd19
remove extremely pointless rule
Rich-Harris Jun 3, 2024
6adfd84
remove another daft rule
Rich-Harris Jun 3, 2024
5ef7290
this is what typescript is for
Rich-Harris Jun 3, 2024
843a9ae
again, typescript already has this covered, we don't need it
Rich-Harris Jun 3, 2024
e22befd
simplify
Rich-Harris Jun 3, 2024
7d5f7bb
this rule is harmless
Rich-Harris Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,21 @@ import lube from 'eslint-plugin-lube';
export default [
...svelte_config,
{
languageOptions: {
parserOptions: {
project: true
}
},
plugins: {
lube
},
rules: {
'@typescript-eslint/await-thenable': 'error',
'@typescript-eslint/no-floating-promises': 'error',
'@typescript-eslint/no-misused-promises': 'error',
'@typescript-eslint/prefer-promise-reject-errors': 'error',
'@typescript-eslint/promise-function-async': 'error',
'@typescript-eslint/require-await': 'error',
'no-console': 'error',
'lube/svelte-naming-convention': ['error', { fixSameNames: true }],
// eslint isn't that well-versed with JSDoc to know that `foo: /** @type{..} */ (foo)` isn't a violation of this rule, so turn it off
Expand Down Expand Up @@ -43,7 +54,6 @@ export default [
'documentation',
// contains a fork of the REPL which doesn't adhere to eslint rules
'sites/svelte-5-preview/**',
'playgrounds/demo/src/**',
'tmp/**',
// wasn't checked previously, reenable at some point
'sites/svelte.dev/**'
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/preprocess/replace_in_code.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export function slice_source(code_slice, offset, { file_basename, filename, get_
* @param {(...match: any[]) => Promise<MappedCode>} get_replacement
* @param {string} source
*/
function calculate_replacements(re, get_replacement, source) {
async function calculate_replacements(re, get_replacement, source) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. What is this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it returns a promise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to be clear: I understand that eslint fails because of @typescript-eslint/promise-function-async, but that rule seems particularly pointless even by the standards of the genre)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in fact i'm pretty sure that making a function async unnecessarily creates runtime overhead. i just have no idea why we would do this to ourselves)

Copy link
Member

@Conduitry Conduitry Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the ones that would still return a promise but don't have an await in them, probably, yeah.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely. But to be honest I question the value of any of it. We need less naggy ESLint bullshit, not more. The fact that you have to add this...

// eslint-disable-next-line @typescript-eslint/no-floating-promises

...to tell the plugin that Promise.resolve() — of all things! — doesn't need a .catch handler tells you all you need to know about how much value it adds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand the benefit of leaving it off

As mentioned, it creates runtime overhead. AFAIK this will result in four promises being created rather than two:

async function get(thing) {
  return fetch(`${base}/${thing}`).then(async (r) => r.json());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...to tell the plugin that Promise.resolve() — of all things! — doesn't need a .catch handler tells you all you need to know about how much value it adds.

I believe that the issue isn't Promise.resolve(), but that an exception could be thrown in .then() and then it would be unhandled

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way it's an annoying rule that we really don't need. I got rid of the ones that don't provide any value (or provide negative value, as in the case of promise-function-async)

/**
* @type {Array<Promise<import('./private.js').Replacement>>}
*/
Expand Down
1 change: 1 addition & 0 deletions packages/svelte/src/internal/client/dom/elements/custom-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ if (typeof HTMLElement === 'function') {
disconnectedCallback() {
this.$$cn = false;
// In a microtask, because this could be a move within the DOM
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Promise.resolve().then(() => {
if (!this.$$cn && this.$$c) {
this.$$c.$destroy();
Expand Down
1 change: 1 addition & 0 deletions packages/svelte/src/internal/client/dom/elements/misc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export function add_form_reset_listener() {
(evt) => {
// Needs to happen one tick later or else the dom properties of the form
// elements have not updated to their reset values yet
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Promise.resolve().then(() => {
if (!evt.defaultPrevented) {
for (const e of /**@type {HTMLFormElement} */ (evt.target).elements) {
Expand Down
6 changes: 4 additions & 2 deletions packages/svelte/src/motion/spring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export function spring(value, opts = {}) {
* @param {import('./private').SpringUpdateOpts} opts
* @returns {Promise<void>}
*/
function set(new_value, opts = {}) {
async function set(new_value, opts = {}) {
target_value = new_value;
const token = (current_token = {});
if (value == null || opts.hard || (spring.stiffness >= 1 && spring.damping >= 1)) {
Expand Down Expand Up @@ -120,6 +120,7 @@ export function spring(value, opts = {}) {
});
}
return new Promise((fulfil) => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
/** @type {import('../internal/client/types').Task} */ (task).promise.then(() => {
if (token === current_token) fulfil();
});
Expand All @@ -128,7 +129,8 @@ export function spring(value, opts = {}) {
/** @type {import('./public.js').Spring<T>} */
const spring = {
set,
update: (fn, opts) => set(fn(/** @type {T} */ (target_value), /** @type {T} */ (value)), opts),
update: async (fn, opts) =>
set(fn(/** @type {T} */ (target_value), /** @type {T} */ (value)), opts),
subscribe: store.subscribe,
stiffness,
damping,
Expand Down
4 changes: 2 additions & 2 deletions packages/svelte/src/motion/tweened.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export function tweened(value, defaults = {}) {
* @param {T} new_value
* @param {import('./private').TweenedOptions<T>} [opts]
*/
function set(new_value, opts) {
async function set(new_value, opts) {
target_value = new_value;

if (value == null) {
Expand Down Expand Up @@ -145,7 +145,7 @@ export function tweened(value, defaults = {}) {
}
return {
set,
update: (fn, opts) =>
update: async (fn, opts) =>
set(fn(/** @type {any} */ (target_value), /** @type {any} */ (value)), opts),
subscribe: store.subscribe
};
Expand Down
1 change: 0 additions & 1 deletion playgrounds/demo/.prettierignore
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions playgrounds/demo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
"ssr": "node ./server.js",
"build": "vite build --outDir dist/client && vite build --outDir dist/server --ssr src/entry-server.ts",
"prod": "npm run build && node dist",
"preview": "vite preview",
"format": "prettier --check --write .",
"lint": "prettier --check . && eslint"
"preview": "vite preview"
},
"devDependencies": {
"@sveltejs/vite-plugin-svelte": "^3.1.0",
Expand Down
4 changes: 3 additions & 1 deletion playgrounds/sandbox/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ function mkdirp(dir) {
const svelte_modules = glob('**/*.svelte', { cwd: `${cwd}/input` });
const js_modules = glob('**/*.js', { cwd: `${cwd}/input` });

for (const generate of ['client', 'server']) {
/** @type {Array<'client'|'server'>} */
const compile_types = ['client', 'server'];
for (const generate of compile_types) {
console.error(`\n--- generating ${generate} ---\n`);
for (const file of svelte_modules) {
const input = `${cwd}/input/${file}`;
Expand Down
2 changes: 1 addition & 1 deletion playgrounds/sandbox/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@
"allowJs": true,
"checkJs": true
},
"include": ["./input"]
"include": ["./run.js", "./input"]
}
Loading