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.

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

allow use before define #9361

Merged
merged 7 commits into from
Nov 20, 2024
Merged

allow use before define #9361

merged 7 commits into from
Nov 20, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented May 13, 2024

no ticket

Description

The no-use-before-define rule was motivated by when variables could hoist up in surprising ways. We always use let and const which have the Temporal Dead Zone that will throw an error if used before declaration.

So we don't need this rule anymore and it's mostly noise.

This also removes no-constant-condition.

Whenever someone wants the rule they can still opt in,

/* eslint no-use-before-define: "error" */

Security Considerations

n/a, lint

Scaling Considerations

n/a, lint

Documentation Considerations

reduces need for docs

Testing Considerations

CI

Upgrade Considerations

n/a, lint

@turadg turadg mentioned this pull request May 13, 2024
Copy link

cloudflare-workers-and-pages bot commented May 13, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: babf0ea
Status: ✅  Deploy successful!
Preview URL: https://08e5fb9a.agoric-sdk.pages.dev
Branch Preview URL: https://ta-allow-use-before-define.agoric-sdk.pages.dev

View logs

@turadg turadg requested review from warner and kriskowal May 13, 2024 17:27
@dckc
Copy link
Member

dckc commented May 13, 2024

... We always use let and const

We haven't removed all use of function. It looks like hundreds of them remain.

https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/controller/controller.js
https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/kernel/kernel.js

function convert(data) {

etc.

I don't think we even have a lint rule to avoid it.

@turadg
Copy link
Member Author

turadg commented May 13, 2024

We haven't removed all use of function. It looks like hundreds of them remain.

Good point. But as you also point out this rule doesn't pertain to those.

Any reason to keep this rule?

@dckc
Copy link
Member

dckc commented May 13, 2024

We haven't removed all use of function. It looks like hundreds of them remain.

Good point. But as you also point out this rule doesn't pertain to those.

use-before-defined does apply to functions, IIUC.

What I meant was: we don't have a rule to say "use const instead of function".

@turadg turadg force-pushed the ta/allow-use-before-define branch from 7ad4373 to c4d7f8d Compare May 13, 2024 19:07
@turadg turadg requested review from erights and removed request for kriskowal May 13, 2024 19:19
packages/governance/package.json Outdated Show resolved Hide resolved
packages/casting/package.json Outdated Show resolved Hide resolved
@turadg turadg force-pushed the ta/allow-use-before-define branch 2 times, most recently from 6f1744c to 31d3f7d Compare May 16, 2024 22:11
@turadg turadg requested a review from erights May 16, 2024 22:11
@turadg turadg force-pushed the ta/allow-use-before-define branch from 31d3f7d to b4fde93 Compare November 13, 2024 20:14
@turadg turadg requested a review from a team as a code owner November 13, 2024 20:14
@turadg turadg force-pushed the ta/allow-use-before-define branch from b4fde93 to 6b8ff51 Compare November 13, 2024 20:39
@turadg turadg changed the title allow use before define allow use before define and constant conditions Nov 13, 2024
@kriskowal
Copy link
Member

I support this change even if we don’t fully migrate from function to const arrow functions. I happen to write a great deal of mutually recursive code that is best explained in the subjectively wrong order, so that is a bias, and I just don’t experience the surprise of using before definition at runtime in practice. I can’t speak for everyone, though.

@dckc
Copy link
Member

dckc commented Nov 13, 2024

Does this mean I can go back to writing code top-down? oh my. whee!

@Chris-Hibbert
Copy link
Contributor

I don't have a problem with these changes to our coding style.

We always use let and const which have the Temporal Dead Zone that will throw an error if used before declaration.

Is use of function rather than let susceptible to the TDZ problem? That seems like an issue.

@erights
Copy link
Member

erights commented Nov 13, 2024

As suggested, #10464 would now globally suppress no-constant-condition, by borrowing that portion of this PR.

@turadg turadg force-pushed the ta/allow-use-before-define branch from 6b8ff51 to f1f96ad Compare November 15, 2024 22:40
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

As agreed verbally. LGTM, thanks!

@turadg turadg changed the title allow use before define and constant conditions allow use before define Nov 20, 2024
@turadg turadg force-pushed the ta/allow-use-before-define branch from f1f96ad to b05fc01 Compare November 20, 2024 16:42
@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 20, 2024
@turadg turadg added the bypass:integration Prevent integration tests from running on PR label Nov 20, 2024
@turadg turadg force-pushed the ta/allow-use-before-define branch from deb29e7 to 1ceed0f Compare November 20, 2024 17:53
@turadg turadg force-pushed the ta/allow-use-before-define branch from 1ceed0f to babf0ea Compare November 20, 2024 18:15
@mergify mergify bot merged commit 59b637f into master Nov 20, 2024
77 checks passed
@mergify mergify bot deleted the ta/allow-use-before-define branch November 20, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants