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

SyntaxError: Invalid or unexpected token 50.toString() #16040

Open
maxmartynov opened this issue Mar 15, 2024 · 6 comments · May be fixed by #16041
Open

SyntaxError: Invalid or unexpected token 50.toString() #16040

maxmartynov opened this issue Mar 15, 2024 · 6 comments · May be fixed by #16041
Labels
domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@maxmartynov
Copy link

📝 Provide detailed reproduction steps (if any)

Some algorithms of minification turn code like this ITEMS_PER_REQUEST.toString() to 50.toString() - substitute the variable's value. This leads to the error: SyntaxError: Invalid or unexpected token.

The place where I personally have this problem

categoryUrl.searchParams.set( 'limit', ITEMS_PER_REQUEST.toString() );

❓ Possible solution

Following the specification of Number.prototype.toString(), using another way of stringifying numbers will be safer. I suggest using String(number)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toString


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@maxmartynov maxmartynov added the type:bug This issue reports a buggy (incorrect) behavior. label Mar 15, 2024
maxmartynov added a commit to maxmartynov/ckeditor5 that referenced this issue Mar 15, 2024
@maxmartynov
Copy link
Author

Fixed in the PR #16041

@Witoso
Copy link
Member

Witoso commented Mar 19, 2024

Ha, that's very intriguing. What kind of bundler/minifier are you using? Tactical cc to @filipsobol, as we start to bundle ourselves in #15502.

@maxmartynov
Copy link
Author

I use Terser (Stack: Nuxt.js 3, Vue 3, Vite, Terser).

Disabling these two options of Terser makes ckeditor build correctly:

export default defineNuxtConfig({
  vite: {
    build: {
      minify: 'terser',
      terserOptions: {
        compress: {
          reduce_vars: false,
          collapse_vars: false,
        },
      },
    },
  },
 })

But in general, I had to disable these two options only for two rows in ckeditor lib which I've fixed in my PR. If I disable these options, the expression ITEMS_PER_REQUEST.toString() becomes 50..toString() (with two dots) in the result bundle and it works

@filipsobol
Copy link
Member

filipsobol commented Mar 19, 2024

We use terser to minify bundles for the new installation methods (#15502) and it correctly converts ITEMS_PER_REQUEST.toString() to 50..toString(). This is consistent with the output in the Terser REPL:

// Input
const test = 50;

console.log(test.toString());

// Output
console.log(50..toString());

@maxmartynov Are you using the latest version of terser?


Out of curiosity, I also checked swc and (when compression is enabled) it converts ITEMS_PER_REQUEST.toString() to "50".

@Witoso Witoso added the domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. label Mar 19, 2024
@Witoso
Copy link
Member

Witoso commented Mar 19, 2024

Hmm, with the options mentioned by @maxmartynov

        compress: {
          reduce_vars: false,
          collapse_vars: false,
        },

the live REPL also produces working code:

const o=50;console.log(o.toString());

It looks like a Terser issue (maybe older version produced wrong results), or maybe something else causes problems in your environment?

TBH I don't see this as a bug on our side 🤔 Changing working code to some other implementation just because minification crashes it, looks like the tail wagging the dog.

@maxmartynov
Copy link
Author

TLDR;
If you guys are sure the new PR will solve this problem - no problem. Feel free to close the issue. You have already paid attention to the problem which is enough to remember it if necessary. My goal was to tell you about a nasty issue that happens on runtime.

Full story

I use ckeditor5@41.2.0. Perhaps this problem will go away with new installation methods (#15502)

@filipsobol

@maxmartynov Are you using the latest version of terser?

I use (Nuxt.js uses under the hood) terser@5.27.0. The latest version is 5.29.2.

it correctly converts ITEMS_PER_REQUEST.toString() to 50..toString()

I confirm that for me disabling the flags reduce_vars: false, collapse_vars: false leads to having 50..toString() in the resulting bundle. And this works.

I've created this issue mostly to point out the fact that (warn: IMHO) using <number>.toString() is not the best practice. My project uses ~70 libs in package.json and that means about 1.4k libs as nested dependencies. And they all play well with these Terser options enabled. The only place I noticed a problem is this one row in ckeditor5. You can check your project and make sure that there are no other places where you stringify numbers this way (not 100% info).

The reason why I've created this issue is to report the problem that appears in runtime only and happens NOT IN 100% CASES. My users got the error SyntaxError: Invalid or unexpected token randomly in different browsers. I assume that some browser engines can handle 50.toString() sometimes, because I don't know the reason why only half of my users get this error sometimes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:integration-dx This issue reports a problem with the developer experience when integrating CKEditor into a system. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants