WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 3 months ago

#44371 closed defect (bug) (fixed)

Make sure all JS globals are explicitly assigned to the window.

Reported by: omarreiss Owned by: omarreiss
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: javascript Cc:

Description

Currently there are a lot of variables in the JavaScript that are being defined in the global scope without being explicitly assigned to the window. When we start using Webpack to build all the JavaScript, the code will get encapsulated in an anonymous function and those implicit globals could get assigned to a different scope. To make sure that doesn't happen, I've created a patch to make sure all globals are explicitly assigned to the window.

Attachments (4)

explicit-globals.patch (21.7 KB) - added by omarreiss 7 months ago.
explicit-globals.2.patch (22.3 KB) - added by omarreiss 5 months ago.
explicit-globals.3.patch (24.7 KB) - added by omarreiss 5 months ago.
explicit-globals.4.patch (23.8 KB) - added by omarreiss 5 months ago.
Explicitly referencing globals on the window is not in scope for this ticket.

Download all attachments as: .zip

Change History (18)

#1 @subrataemfluence
7 months ago

  • Keywords has-patch needs-testing added

#2 @netweb
7 months ago

  • Milestone changed from Awaiting Review to 5.0

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-js by aduth. View the logs.


7 months ago

#5 @pento
6 months ago

  • Keywords commit added; needs-testing removed
  • Owner set to omarreiss
  • Status changed from new to assigned

Let's get this in.

All of these globals appear to be intended to be globally scoped: they're all declared outside the functions where they're used later in the files.

#6 @omarreiss
5 months ago

Patch updated, went through everything again, found one more implicit global. Plan on committing this today.

#7 @pento
5 months ago

🕺🏻

@omarreiss
5 months ago

Explicitly referencing globals on the window is not in scope for this ticket.

#8 @omarreiss
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 43577:

General: Explicitly assigns all JS globals to the window.

Many variables in the JavaScript were defined in the global scope without being explicitly assigned to the window. When built with Webpack, the code gets encapsulated in anonymous functions and those implicit globals get assigned to the wrong scope. This patch prevents that from happening.

Fixes #44371. See #43731.

#9 @azaozz
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[43577] broke quicktags.js. Also some of the changes there don't seem to make much sense, especially for a very very VERY old lib like QuickTags :)

Last edited 5 months ago by azaozz (previous) (diff)

#10 @azaozz
5 months ago

In 43579:

Reorder quicktags.js a bit to account for the order of defining vars and functions after [43577].

See #44371.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


5 months ago

#12 follow-up: @pento
3 months ago

  • Keywords close added; has-patch commit removed
  • Milestone changed from 5.0 to 5.1

@azaozz: Was [43579] the only change that needed to happen here?

#13 in reply to: ↑ 12 @azaozz
3 months ago

Replying to pento:

@azaozz: Was [43579] the only change that needed to happen here?

Yeah, assigning of the functions to vars in [43577] broke the order in quicktags.js a bit. The rest seems to be working.

Example (paste in the console):

test();
function test() {
 console.log(1);
}
// 1

window.test2();
window.test2 = function() {
 console.log(2);
}
// ReferenceError: test2 is not defined.
Last edited 3 months ago by azaozz (previous) (diff)

#14 @pento
3 months ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from reopened to closed

🕺🏻

Note: See TracTickets for help on using tickets.