CodinGame / monaco-vscode-api

VSCode public API plugged on the monaco editor
MIT License
208 stars 29 forks source link

Paste functionality not working #377

Closed char0n closed 3 months ago

char0n commented 3 months ago

Here is a minimal code to reproduce.

import { initialize as initializeMonacoServices } from 'vscode/services';
import 'vscode/localExtensionHost';

wait initializeMonacoServices({});

When I render a monaco editor, I've noticed that the paste functionality is not working at all. It's impossible to paste a text to the editor. Probably the code that handles pasting got removed by accident or become part of other codingame service override.

char0n commented 3 months ago

Might be related to https://github.com/microsoft/monaco-editor/issues/4438

char0n commented 3 months ago

I confirmed that node_modules/vscode/vscode/src/vs/editor/contrib/clipboard/browser/clipboard.js is executed in browser and that https://github.com/microsoft/monaco-editor/issues/4438 is the source of the problem.

char0n commented 3 months ago

This brings me to the realization that with @codingame/monaco-vscode-editor-api it is not clear at all what version of monaco-editor this represents and how to match issues from https://github.com/microsoft/monaco-editor/issues to releases of @codingame/monaco-vscode-editor-api

CGNonofr commented 3 months ago

You should ensure that process is properly not defined

This brings me to the realization that with @codingame/monaco-vscode-editor-api it is not clear at all what version of monaco-editor this represents and how to match issues from https://github.com/microsoft/monaco-editor/issues to releases of @codingame/monaco-vscode-editor-api

We are synchonized on the VSCode releases and not on the editor releases anymore, but they often happen really close in time

char0n commented 3 months ago

We are synchonized on the VSCode releases and not on the editor releases anymore, but they often happen really close in time

Thanks for explanation.

For the time being I'll try to downgrade below the affected versions. Following versions are affected by the issue:

    "monaco-editor": "npm:@codingame/monaco-vscode-editor-api@=3.2.2",
    "vscode": "npm:@codingame/monaco-vscode-api@=3.2.2",
CGNonofr commented 3 months ago

We are synchonized on the VSCode releases and not on the editor releases anymore, but they often happen really close in time

Thanks for explanation.

For the time being I'll try to downgrade below the affected versions. Following versions are affected by the issue:

    "monaco-editor": "npm:@codingame/monaco-vscode-editor-api@=3.2.2",
    "vscode": "npm:@codingame/monaco-vscode-api@=3.2.2",

Are you sure it's an issue for you to not inject process though? it's not a good practice anyway

char0n commented 3 months ago

Are you sure it's an issue for you to not inject process though? it's not a good practice anyway

I'm already injecting process because I need it elsewhere, so I cannot just set it to undefined to get around the bug.

      new webpack.ProvidePlugin({
        process: 'process/browser.js',
      }),
char0n commented 3 months ago

Would it be possible to apply a patch as part of this repo build process?

Update: having said that I realize that this repo is not a place to fix upstream bugs

char0n commented 3 months ago

Found a workaround for this bug by utilizing default webpack BannerPlugin:

      new webpack.BannerPlugin({
        banner: "globalThis.vscode = { process: Symbol.for('vscode') };",
        raw: true, // This is important, it tells webpack to prepend the code as-is.
        entryOnly: true // This adds the banner only to the beginning of the bundle.
      }),

globalThis.vscode = { process: Symbol.for('vscode') }; - works as a workaround for the new flow logic introduced in https://github.com/microsoft/vscode/pull/200935/files

The above code is prepended before every entry point and we're making sure that downstream projects don't event need to know about this.

CGNonofr commented 3 months ago

Would it be possible to apply a patch as part of this repo build process?

Unfortunately, the library can be used with electron and that change will break it

Update: having said that I realize that this repo is not a place to fix upstream bugs

You seem to considering it as a bug while it's not really a bug. You're not supposed to have a process defined in the browser.

Found a workaround for this bug by utilizing default webpack BannerPlugin:

      new webpack.BannerPlugin({
        banner: "globalThis.vscode = { process: Symbol.for('vscode') };",
        raw: true, // This is important, it tells webpack to prepend the code as-is.
        entryOnly: true // This adds the banner only to the beginning of the bundle.
      }),

globalThis.vscode = { process: Symbol.for('vscode') }; - works as a workaround for the new flow logic introduced in https://github.com/microsoft/vscode/pull/200935/files

The above code is prepended before every entry point and we're making sure that downstream projects don't event need to know about this.

I'm not sure how to manage to make isWeb = true with that change

char0n commented 3 months ago

You seem to considering it as a bug while it's not really a bug. You're not supposed to have a process defined in the browser.

I need have the process defined due to other required dependencies withing the dependency tree.

I'm not sure how to manage to make isWeb = true with that change

It works because of this code.

let nodeProcess: INodeProcess | undefined = undefined;
if (typeof $globalThis.vscode !== 'undefined' && typeof $globalThis.vscode.process !== 'undefined') {
    // Native environment (sandboxed)
    nodeProcess = $globalThis.vscode.process;
} else if (typeof process !== 'undefined') {
    // Native environment (non-sandboxed)
    nodeProcess = process;
}

Then it goes to

if (typeof nodeProcess === 'object') {

and finally to:

else if (typeof navigator === 'object' && !isElectronRenderer) {

...which is what I want.

char0n commented 3 months ago

Closing as my issue is not really specific to this repository. Thanks for help!

CGNonofr commented 3 months ago

I need have the process defined due to other required dependencies withing the dependency tree.

Then it's a bug of the other library, or it's not compatible with the browser

It works because of this code.

let nodeProcess: INodeProcess | undefined = undefined;
if (typeof $globalThis.vscode !== 'undefined' && typeof $globalThis.vscode.process !== 'undefined') {
  // Native environment (sandboxed)
  nodeProcess = $globalThis.vscode.process;
} else if (typeof process !== 'undefined') {
  // Native environment (non-sandboxed)
  nodeProcess = process;
}

Then it goes to

if (typeof nodeProcess === 'object') {

and finally to:

else if (typeof navigator === 'object' && !isElectronRenderer) {

...which is what I want.

I've tried this kind of hacks and I had issues because global.vscode.process is also used in vs/base/common/process.ts with a much simpler condition, and your change makes platform undefined and a lot of stuff uses that, including the path management, making the window detection not working

char0n commented 3 months ago

Good point. I need to address the root cause...

char0n commented 3 months ago

Unfortunately I'm unable to address the root cause because of the fact that AsyncAPI renderer (which I'm using) is using https://github.com/APIDevTools/json-schema-ref-parser.

It's using a process.prop without any imports. So that's why ProvidePlugin must be used to build the project.

CGNonofr commented 3 months ago

I see no code in https://github.com/APIDevTools/json-schema-ref-parser that uses the process object without checking it before in the browser, what file are you referring to?

char0n commented 3 months ago

https://github.com/APIDevTools/json-schema-ref-parser/blob/7a8806da56a852e28df66d45a2238faa5b3bcf84/lib/util/url.ts#L50

CGNonofr commented 3 months ago

It's only called if window is undefined

btw, their documentation mentions webpack fallback but nothing about process

char0n commented 3 months ago

Yeah, its because I'm using version v9 and v11 doesn't have this issue.

char0n commented 3 months ago

I was able to apply process import just for a single file using webpack rule:

            {
              test: /@apidevtools\/json-schema-ref-parser\/lib\/util\/url.js$/,
              loader: 'imports-loader',
              options: {
                type: 'commonjs',
                imports: ['single process/browser process'],
              },
            },

Unfortunately this needs to be done by all downstream consumers until I'm able to update the library.

CGNonofr commented 2 months ago

Note that the issue was fixed in https://github.com/microsoft/vscode/commit/8caaab7a3e63b315dbde77cb159cc7928accf8e2

char0n commented 1 month ago

@CGNonofr thanks for the notice