Skip to content
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

Jump to bottom

ESLint await rule cleanup #108

Merged
merged 4 commits into from
Jun 1, 2023
Merged

ESLint await rule cleanup #108

merged 4 commits into from
Jun 1, 2023

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented May 29, 2023 •
edited
Loading

Closes #107

Separate the recommended @jessie.js/safe-await-separator rule from the @jessie.js/no-nested-await rule (now activated by // @jessie-check).

This should be the final step before upgrading the @jessie.js/eslint-plugin package used by the Agoric SDK.

@michaelfig michaelfig self-assigned this May 29, 2023
@michaelfig michaelfig requested a review from mhofman May 29, 2023 21:56
@michaelfig michaelfig marked this pull request as ready for review May 29, 2023 21:57
@michaelfig michaelfig requested a review from turadg May 29, 2023 21:57
turadg
turadg reviewed May 30, 2023
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

This should be the final step before upgrading … agoric-sdk

To be sure, let's run this over that repo and confirming that the the only errors are ones we want to see. If you're busy I could maybe to it tomorrow after RC cut.

packages/eslint-plugin/lib/rules/safe-await-separator.js
context.report({
node,
messageId: 'unsafeAwaitSeparator',
suggest: [
Copy link
Member

Choose a reason for hiding this comment

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

nice!

packages/eslint-plugin/lib/tools.js Outdated

'Program',
'StaticBlock',
].includes(node.type);
Copy link
Member

Choose a reason for hiding this comment

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

this makes the array each time the function runs. not terribly expensive but this function gets called very every node. consider defining the array at module scope.

or does V8 optimize this somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I'll do that. (Regardless of whether V8 does anything magical.)

packages/eslint-plugin/lib/tools.js
'StaticBlock',
].includes(node.type);

const isAwaitAllowedInNode = (node, already = new WeakSet()) => {
Copy link
Member

Choose a reason for hiding this comment

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

mind adding a type signature? would be helpful to document the node type and to have an assurance that it always returns boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely!

packages/eslint-plugin/lib/tools.js
* reveals some of the relevant AST shapes (note that node.parent is not
* displayed in the ASTExplorer, but is available in the ESLint visitor):
*
* @see https://astexplorer.net/#/gist/4508eec25a8d5be1e0248c4cc06b9634/f6b22f2e8e3abd82a911ca6286a304ef0a3018c4
Copy link
Member

Choose a reason for hiding this comment

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

👏

packages/eslint-plugin/test/test-safe-await-separator.js Outdated
clearlyInvalid,
} = require('./corpus.js');

const unsafeAwaitSeparator = 'unsafeAwaitSeparator';
Copy link
Member

Choose a reason for hiding this comment

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

surprised to see this string defined as const. since this is a test I think it's appropriate to inline the literal. if you prefer not to, please do the same for 'unexpectedNestedAwait'

Copy link
Member Author

Choose a reason for hiding this comment

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

Vestiges of an earlier commit. Done!

@michaelfig
Copy link
Member Author

To be sure, let's run this over that repo and confirming that the the only errors are ones we want to see.

I did this with Agoric/agoric-sdk#7870 . It looked good to me.

If you want to reproduce my results:

(cd Jessie/packages/eslint-plugin && yarn link)
cd agoric-sdk
yarn link @jessie.js/eslint-plugin
yarn lint

turadg
turadg approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Looking forward to this QoL improvement :)

@michaelfig michaelfig enabled auto-merge June 1, 2023 20:14
@michaelfig michaelfig merged commit 36c9c20 into main Jun 1, 2023
@michaelfig michaelfig deleted the mfig-107-eslint-await branch June 1, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turadg turadg turadg approved these changes

@mhofman mhofman Awaiting requested review from mhofman

Assignees

@michaelfig michaelfig

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESLint: add @jessie.js/safe-await-separator to plugin:@jessie.js/recommended
2 participants
@michaelfig @turadg