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

window.matchMedia is not a function errors after ugprade to 41.4.1 #16368

Closed
alexander-schranz opened this issue May 16, 2024 · 12 comments
Closed
Assignees
Labels
resolution:resolved This issue was already resolved (e.g. by another ticket). squad:collaboration Issue to be handled by the Collaboration team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented May 16, 2024

πŸ“ Provide detailed reproduction steps (if any)

  1. Run CKEditor in a CLI Environment without access to window like Jest (in our Jest 24)

βœ”οΈ Expected result

What is the expected result of the above steps?

❌ Actual result

 TypeError: window.matchMedia is not a function

      2 | import React from 'react';
      3 | import log from 'loglevel';
    > 4 | import AlignmentPlugin from '@ckeditor/ckeditor5-alignment/src/alignment';
        | ^
      5 | import BoldPlugin from '@ckeditor/ckeditor5-basic-styles/src/bold';
      6 | import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
      7 | import EssentialsPlugin from '@ckeditor/ckeditor5-essentials/src/essentials';

      at isMediaForcedColors (node_modules/@ckeditor/ckeditor5-utils/src/env.js:131:19)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-utils/src/env.js:34:26)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-utils/src/index.js:8:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-core/src/plugin.js:9:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-core/src/index.js:8:1)
      at Object.<anonymous> (node_modules/ckeditor5/src/core.js:8:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-alignment/src/alignment.js:8:1)
      at Object.<anonymous> (src/Sulu/Bundle/AdminBundle/Resources/js/containers/CKEditor5/CKEditor5.js:4:1)
      at Object.<anonymous> (src/Sulu/Bundle/AdminBundle/Resources/js/containers/CKEditor5/index.js:2:1)
      at Object.<anonymous> (src/Sulu/Bundle/AdminBundle/Resources/js/containers/index.js:2:1)
      at Object.<anonymous> (src/Sulu/Bundle/PageBundle/Resources/js/containers/TeaserSelection/tests/TeaserSelection.test.js:5:1)

❓ Possible solution

For server side renderers and cli best rendering it would be best if the window.matchMedia is only access when windowvariable is available.

πŸ“ƒ Other details

  • Browser: none
  • OS: Mac and Linux
  • First affected CKEditor version: think 41.4.1 (CI did run yesterday but today not)
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

@alexander-schranz alexander-schranz added the type:bug This issue reports a buggy (incorrect) behavior. label May 16, 2024
@Reinmar
Copy link
Member

Reinmar commented May 16, 2024

Thanks for reporting.

It's indeed a regression. We should not block importing the entire package in an env like Jest.

The problem is here:

https://github.com/ckeditor/ckeditor5/blob/78174de1c2ddf9f53c76aef903e73f05f877b0f2/packages/ckeditor5-utils/src/env.ts#L112-L113

And it should probably be a getter like this one:

https://github.com/ckeditor/ckeditor5/blob/78174de1c2ddf9f53c76aef903e73f05f877b0f2/packages/ckeditor5-utils/src/env.ts#L114-L116

@Reinmar
Copy link
Member

Reinmar commented May 16, 2024

We'll try to publish a hotfix release tomorrow.

@Reinmar Reinmar added the type:regression This issue reports a bug that was not present in the previous releases. label May 16, 2024
@alexander-schranz
Copy link
Contributor Author

@Reinmar Thank you for the fast response!

@Reinmar
Copy link
Member

Reinmar commented May 16, 2024

Two questions:

  • Will the change to getter resolve this specific problem?
  • Didn't we (by coincident) introduce more issues like this one, preventing importing a specific module in a server-side setting?

Bonus thing: We should such scenarios this to our E2E tests (and cover as many packages as actually work today). cc @Mgsy @dufiplΒ 

@alexander-schranz Do you know about any of our packages which would not be importable in Jest already?

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented May 16, 2024

For me it looks like all plugins try to load node_modules/ckeditor5/src/core.js (import { Plugin } from 'ckeditor5/src/core.js';) which run into that error.

Stack trace:

      at isMediaForcedColors (node_modules/@ckeditor/ckeditor5-utils/src/env.js:131:19)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-utils/src/env.js:34:26)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-utils/src/index.js:8:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-core/src/plugin.js:9:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-core/src/index.js:8:1)
      at Object.<anonymous> (node_modules/ckeditor5/src/core.js:8:1)
      at Object.<anonymous> (node_modules/@ckeditor/ckeditor5-alignment/src/alignment.js:8:1)
      ```

@Reinmar
Copy link
Member

Reinmar commented May 16, 2024

What about before v41.4.1? Did you have to work around any similar issue in any other package?

@alexander-schranz
Copy link
Contributor Author

@Reinmar as a workaround I found out I could create in Jest a instance of window.matchMedia this way but requires to change the setup script of jest that it is available globally: https://jestjs.io/docs/29.4/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented May 16, 2024

Most similar issue with the jest part was this one I think: #12972

Other packages currently was mostly more issues with using a too new javascript syntax which we mostly then fixed babelify also the node_modules package itself by extend: https://github.com/sulu/sulu/blob/67057da5ca0c7c7c3618a6a2ac22a42f61f9e42a/webpack.config.js#L93 with the different packages.

@scofalik
Copy link
Contributor

@alexander-schranz I'd like to make sure that this is the only error we introduced in this release. Could you please confirm if after mocking window.matchMedia you are able to run tests in Jest successfully?

@alexander-schranz
Copy link
Contributor Author

@scofalik yes the window.matchMedia was in our project the only case which did fail.

scofalik added a commit that referenced this issue May 16, 2024
Fix (utils): Prevented error thrown when editor files are imported in an environment without `window` global object. Closes #16368.
@scofalik
Copy link
Contributor

scofalik commented May 16, 2024

This issue should be fixed on release branch.

@Reinmar Reinmar added the squad:collaboration Issue to be handled by the Collaboration team. label May 16, 2024
@Reinmar
Copy link
Member

Reinmar commented May 19, 2024

Released in v41.4.2: https://github.com/ckeditor/ckeditor5/releases/tag/v41.4.2

@Witoso Witoso added the resolution:resolved This issue was already resolved (e.g. by another ticket). label May 20, 2024
@Reinmar Reinmar added this to the iteration 74 (v41.4.1) milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution:resolved This issue was already resolved (e.g. by another ticket). squad:collaboration Issue to be handled by the Collaboration team. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

No branches or pull requests

4 participants