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

Use === instead of == #4924

Closed
5 tasks
cpcallen opened this issue Jun 16, 2021 · 2 comments
Closed
5 tasks

Use === instead of == #4924

cpcallen opened this issue Jun 16, 2021 · 2 comments
Assignees
Labels
component: devtools issue: docs Describes missing or incorrect documentation type: cleanup

Comments

@cpcallen
Copy link
Contributor

This is a tracking bug for moving the Blockly codebase to use === instead of == for comparison (in most cases).

Background

The Blockly codebase originates from a time when it was common practice at Google to use the equality operator (==) rather than the strict equality operator (===) for comparison. The reason for this is somewhat obscure but may simply be that == was introduced in the earliest versions of JavaScript (certainly predating the 1997 ECMAScript 1.0 standard), whereas === was introduced in ECMAScript 1.3 in 1999.

In any case, the version of the Google JavaScript styleguide in use at the time of Blockly's creation, targeting ES5.1, does not weigh in on the issue and in fact does not mention === at all, whereas the current version specifies:

5.10 Equality Checks

Use identity operators (===/!==) except in the cases documented below.

5.10.1 Exceptions Where Coercion is Desirable

Catching both null and undefined values:

if (someObjectOrPrimitive == null) {
  // Checking for null catches both null and undefined for objects and
  // primitives, but does not catch other falsy values like 0 or the empty
  // string.
}

See this Stack Overflow post for more information on === vs. ==.

Proposal

Given that the future of Blockly doubtless involves an eventual transition to ES6+ and/or TypeScript and therefore the newer styleguides (JS, TS), I suggested that we consistently use use === for all new and revised code. In a recent internal team discussion this was met with general consensus, so I therefore propose the following plan:

At the beginning of 2021 Q3 we:

  • Post a notice to the effect that new code (and code being reworked) should use === from now on (except optionally in the cases noted above) in:
  • Configure the linter to reject use of == except when comparing against null or undefined.

As time allows:

  • Replace existing usage of == with ===. This will mostly be a bulk search-and-replace, but code will need to be examined to ensure that where we were previously depending on =='s type conversions an appropriate manual change is made.
@cpcallen cpcallen added component: devtools issue: docs Describes missing or incorrect documentation type: cleanup issue: triage Issues awaiting triage by a Blockly team member labels Jun 16, 2021
@moniika moniika added this to the 2021_q3_2021 milestone Jun 16, 2021
@moniika moniika removed the issue: triage Issues awaiting triage by a Blockly team member label Jun 16, 2021
@NeilFraser
Copy link
Contributor

Historical context: While Google's old style guide was technically silent on the matter, all examples in the style guide are ==, whereas === doesn't appear once. Likewise, all Google code at the time (including Closure) exclusively used == where possible. The reasons for this are discussed here: https://neil.fraser.name/news/2017/06/07/

However, the rest of the JS world chose a different path to Google's, and eventually Google was forced to change.

jschanker added a commit to jschanker/blockly that referenced this issue Jul 8, 2021
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
jschanker added a commit to jschanker/blockly that referenced this issue Jul 8, 2021
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Error now thrown when calling `a.setParent(null)` if a is connected to a superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
jschanker added a commit to jschanker/blockly that referenced this issue Jul 8, 2021
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Fixed lint errors.
jschanker added a commit to jschanker/blockly that referenced this issue Jul 8, 2021
* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per google#4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Fixed lint errors.
* Adjusted comment.
BeksOmega pushed a commit that referenced this issue Jul 13, 2021
* Fix error conditions for setParent #4989

* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).

* Fix error conditions for setParent #4989

* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Error now thrown when calling `a.setParent(null)` if a is connected to a superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).

* Fix error conditions for setParent #4989

* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Fixed lint errors.

* Fix error conditions for setParent #4989

* Error now thrown when calling `a.setParent(b)` on non-null b if the output/previous connection of a is not connected to an input/next connection of b without removing block from old parent's child list.
* Commented out error for when calling `a.setParent(null)` if a is connected to superior block.
* Adjusted comment to reflect that blocks were no longer being disconnected in this method.
* Also changed == to === per #4924 (`newParent` and `this.parentBlock_` must both be instances of `Blockly.Block`).
* Fixed lint errors.
* Adjusted comment.

* Removed unnecessary set to null/added tests

* One is failing (commented out), will investigate later

* Lint fix

* Removed failing test that correctly fails

* Update comments to conform to style guide

Capitalize first letter, period at end
@NeilFraser NeilFraser self-assigned this Oct 13, 2021
@NeilFraser
Copy link
Contributor

Fixed in #5599.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: devtools issue: docs Describes missing or incorrect documentation type: cleanup
Projects
None yet
Development

No branches or pull requests

4 participants