Disallow the use of global in the JS code intended for the browser

[Reinmar]

We've just stumbled upon an issue with the accidental use of global in our code: dc25921

Which resulted in an error only when the editor was built with Vite (and probably other bundlers that don't try to polyfill the CJS env):

The issue in our code was hard to spot during the review because as you can see in dc25921 we actually have a module named global too and when you copy&paste code between files you may copy the usage of it but miss the import.

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[LukaszGudel]

The issue occurred again. Initially reported in ckeditor/vite-plugin-ckeditor5#17.

Using 39.x packages with Vite results in the following error on editor focus:

Uncaught ReferenceError: global is not defined
    at getelementsintersectionrect.js:21:41
    at Array.map (<anonymous>)
    at getElementsIntersectionRect (getelementsintersectionrect.js:18:35)
    at StickyPanelView.checkIfShouldBeSticky (stickypanelview.js:116:38)
    at StickyPanelView.<anonymous> (stickypanelview.js:97:18)
    at StickyPanelView.fire (emittermixin.js:146:47)
    at StickyPanelView.set [as isActive] (observablemixin.js:72:30)
    at updateBoundObservableProperty (observablemixin.js:548:32)
    at observablemixin.js:574:25
    at Set.forEach (<anonymous>)

Source maps point to the

ckeditor5/packages/ckeditor5-utils/src/dom/getelementsintersectionrect.ts

Line 27 in 94a9771

const windowRect = new Rect( global.window );

[filipsobol]

Our first idea to solve this problem was to add an ESLint rule that would disallow the use of global. However, this would only fix the effects, not the source of the problem.

TypeScript configuration

As it turned out, the source of the problem was in-direct dependencies (dependencies of dependencies) in our project that depended on @types/node and other @types packages. Since these packages are automagically loaded, the types from them were available in our project.

To fix this, I created PR #14808 which adds compilerOptions.types = [] to our tsconfig.json to disable automatic loading of types (except those used directly in our code).

ESLint configuration

On top of that, I noticed that the environment was not set up in the ESLint configuration, forcing us to use magic /* globals XXX */ comments in every file where we used environment specific APIs like window, document, setTimeout and so on. To fix this, I created another two more PRs (#14806 and https://github.com/cksource/ckeditor5-commercial/pull/5452).

Code

Finally, I noticed that the utils package exports a variable called global, which is used to access window and document. Comments suggested that this was a hack to mock window and document in sinon.

After some testing it turns out that this is no longer needed and direct access to window and document works just fine, so I've created two more PRs (#14818 and https://github.com/cksource/ckeditor5-commercial/pull/5461) to remove global from utils.

Since this is a breaking change, we'll (1) write an ADR to get feedback from the community and other teams, and (2) wait with these two PRs until we start working on the v40 release.

[Witoso]

If I could propose something, I suggest creating an issue for each PR. There's so many of them, that it becomes unclear what closes what issue and in which order. We should make it more visible on our boards.

[niegowski]

ESLint configuration

On top of that, I noticed that the environment was not set up in the ESLint configuration, forcing us to use magic /* globals XXX */ comments in every file where we used environment specific APIs like window, document, setTimeout and so on. To fix this, I created another two more PRs (#14806 and cksource/ckeditor5-commercial#5452).

Those comments could indicate where we use DOM APIs if we ever want to support headless editor mode (running without DOM on the node server). If we remove those, then it could be harder to spot. OTOH we could then switch to node env and check what is failing, but this would need to scan much more errors and could be overwhelming.

[niegowski]

Code

Finally, I noticed that the utils package exports a variable called global, which is used to access window and document. Comments suggested that this was a hack to mock window and document in sinon.

After some testing it turns out that this is no longer needed and direct access to window and document works just fine, so I've created two more PRs (#14818 and cksource/ckeditor5-commercial#5461) to remove global from utils.

Since this is a breaking change, we'll (1) write an ADR to get feedback from the community and other teams, and (2) wait with these two PRs until we start working on the v40 release.

I feel like this could be related to my previous comment (for easier moving to a headless editor).

[dpawlowski-cksource]

Hello guys, thanks to @LukaszGudel we noticed that you plan to remove global usage from ckeditor5-utils. We are using it in cs-client in few files e.g. here.

As we are using cs-client in our E2E tests (running constantly every 5 min) that are launched in Node.js (which doesn't have access to DOM elements such as window, document) so far it was enough to use this trick.

However, we are not sure if this will still work after removing global completely.

Are you able to release for us some test version of ckeditor5 that we could use in cs-client to test whether we will be able to use it in our E2E tests (Node.js) after your changes?

---

It would also be ideal to get an editorBundle that we can test to see if it works with features like documents-storage. However, as these functions are already run in the browser (puppeteer), we do not anticipate problems with this.

[filipsobol]

As @Witoso suggested, let's move the discussion about updating eslint configuration and removing global from utils to separate issues.

ESLint configuration: #14860.
global in utils: #14861.

Issue raised in this issue will be fixed by PR #14808.