TypeFox / monaco-components

Monaco Editor and Language Client Wrapper, plus Monaco Editor React Component
MIT License
42 stars 13 forks source link

Allow to init and start separately. #59

Closed kaisalmen closed 9 months ago

kaisalmen commented 10 months ago

This allow to init and start the wrapper in two steps (from #56). Using just start is still considered the default. This changes enforces to dispose otherwise init will throw an exception.

This is integrated in the react component as well now. Most method are now protected, so a derived component can override the handleReinit, destroyMonaco, initMonaco or startMonaco.

kaisalmen commented 10 months ago

New next versions are available: https://www.npmjs.com/package/monaco-editor-wrapper/v/3.4.0-next.2 https://www.npmjs.com/package/@typefox/monaco-editor-react/v/2.4.0-next.2

These version use monaco-languageclient@7.0.0 and therefore obey: https://github.com/TypeFox/monaco-languageclient#new-with-v7-treemended-monaco-editor

kaisalmen commented 10 months ago

@cdietrich your opinion or review of the react component changes would be helpful. 🙂

cdietrich commented 10 months ago

will need to when when i find time to look. maybe tomorrow

kaisalmen commented 10 months ago

will need to when when i find time to look. maybe tomorrow

No rush, sounds good. 👍

cdietrich commented 10 months ago

in a first step i just bumped. this gives

[vite]: Rollup failed to resolve import "monaco-editor/esm/vs/platform/remote/common/remoteHosts.js" from "/Users/dietrich/git/my-monaco-editor-react-example/node_modules/vscode/vscode/src/vs/platform/telemetry/common/telemetryUtils.js".

any idea?

cdietrich commented 10 months ago

npm run dev gives

 Could not resolve "monaco-editor/esm/vs/platform/userDataProfile/common/userDataProfile.js"

    node_modules/@codingame/monaco-vscode-configuration-service-override/configuration.js:14:41:
      14 │ ...serDataProfilesService } from 'monaco-editor/esm/vs/platform/userDataProfile/common/userDataProfile.js';
         ╵                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.....
  You can mark the path "monaco-editor/esm/vs/platform/userDataProfile/common/userDataProfile.js" as
  external to exclude it from the bundle, which will remove this error.

✘ [ERROR] Could not resolve "monaco-editor/esm/vs/platform/remote/common/remoteHosts.js"

...
kaisalmen commented 10 months ago

@cdietrich did you check this (from second comment):

These version use monaco-languageclient@7.0.0 and therefore obey: https://github.com/TypeFox/monaco-languageclient#new-with-v7-treemended-monaco-editor

cdietrich commented 10 months ago

i have these in my package.json. shouldnt that fullfill it

  "dependencies": {
    "@codingame/monaco-vscode-configuration-service-override": "^1.83.5",
    "@codingame/monaco-vscode-editor-service-override": "^1.83.5",
    "@codingame/monaco-vscode-files-service-override": "^1.83.5",
    "@typefox/monaco-editor-react": "^2.4.0-next.2",
    "monaco-editor-workers": "^0.44.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "vscode": "npm:@codingame/monaco-vscode-api@^1.83.5"
  },
kaisalmen commented 10 months ago

Now you need to define the overrides for any sub-dependencies to be inline. Just do it as done here: https://github.com/TypeFox/monaco-components/blob/main/packages/examples/package.json

resolutions is only for yarn. overrides work with npm/pnpm

cdietrich commented 10 months ago

ok, is there an easy way to find out which service package has which missing dependency

cdietrich commented 10 months ago

just adding the overrides and resolutions does not help see https://github.com/cdietrich/my-monaco-editor-react-example/compare/cd/700test

kaisalmen commented 10 months ago

"monaco-editor": "$monaco-editor" is a reference to a defined dependency. You have a defined dependency for vscode but not for monaco-editor. Either fully specify in overrides or define it in dependencies: "monaco-editor": "npm:@codingame/monaco-editor-treemended@>=1.83.5 <1.84.0",

When I do that it works

kaisalmen commented 10 months ago

ok, is there an easy way to find out which service package has which missing dependency

missing is not the problem. mismatch versions are the problem. Sorry, this produces another hick-up, but the other approach was worse, because it the patching/treemending monaco-editor was hidden from the user/project. Now it is explicitly required/stated

cdietrich commented 10 months ago

problem is that it still does not work. unfortunately i am still and noob in the npm ecosystem so i have. no clue what is happening

kaisalmen commented 10 months ago

unfortunately i am still and noob in the npm ecosystem so i have. no clue what is happening

It is a mess. I am moving between noob or expert every day. 😆

Remove everything from your checkout (git clean -f -X -d) and do it again. That is my only change.

diff --git a/package.json b/package.json
index 3bee246..4494b8d 100644
--- a/package.json
+++ b/package.json
@@ -15,6 +15,7 @@
     "@codingame/monaco-vscode-files-service-override": "^1.83.5",
     "@typefox/monaco-editor-react": "^2.4.0-next.2",
     "monaco-editor-workers": "^0.44.0",
+    "monaco-editor": "npm:@codingame/monaco-editor-treemended@>=1.83.5 <1.84.0",
     "react": "^18.2.0",
     "react-dom": "^18.2.0",
     "vscode": "npm:@codingame/monaco-vscode-api@^1.83.5"
cdietrich commented 10 months ago

yes -fdx helped me too. thx. now i can start testing my actual stuff later today or 2morrow

kaisalmen commented 10 months ago

yes -fdx helped me too.

I add that as npm script in most projects now.

cdietrich commented 10 months ago

i wonder if we can make the mustReInit accessible.

or something else where i can hook in and have access to this.getEditorWrapper().getLanguageClient() (any maybe old props) so that i can send messages to the server

maybe optional parameter on onLoad callback?

kaisalmen commented 10 months ago

i wonder if we can make the mustReInit accessible.

Good idea. I will do that

cdietrich commented 10 months ago

good with that i can check for changes of otherFileContent or otherFileUri here and my startMonaco override will be called. still need to find out how i access the old props in startMonaco so that i can close them. maybe can use some state to store it. right now i workaround this by setting a key with date on parent component but of course this will lead to needless mounts

      <MonacoEditorReactCompExtended
    // key={new Date().toISOString() /* without this the updated monaco seems to not usable at all */}  
    userConfig={userConfig}
    onLoad={onLoad}
    otherFileContent={otherFileContent}
    otherFileUri={otherFileUri}
    style={{
      paddingTop: '5px',
      height: '40vh',
      width: '100%',
    }}

        />
kaisalmen commented 10 months ago

Pushed an update an new next versions: https://www.npmjs.com/package/monaco-editor-wrapper/v/3.4.0-next.3 https://www.npmjs.com/package/@typefox/monaco-editor-react/v/2.4.0-next.3

cdietrich commented 10 months ago

cool that worked. as monaco will be started i dont need to close old docs

cdietrich commented 10 months ago

i still see something i dont understand. why in my example grafik already returns true

cdietrich commented 10 months ago

improper compare of objects? grafik

kaisalmen commented 10 months ago

@cdietrich can I reproduce this with your above repo?

cdietrich commented 10 months ago

Yea if you use the recently updated branch

kaisalmen commented 10 months ago

@cdietrich I can re-produce the issue with your example. The comparison only works properly for simple properties. That should be re-producible by a unit test (=fix template). Will do it with this PR, too. Thank your for spotting this.

kaisalmen commented 10 months ago

As you can see the test fails now ⬇️

kaisalmen commented 10 months ago

@cdietrich The comparison problem is resolved: https://www.npmjs.com/package/monaco-editor-wrapper/v/3.4.0-next.4 https://www.npmjs.com/package/@typefox/monaco-editor-react/v/2.4.0-next.4

cdietrich commented 10 months ago

nice and thanks. works fine

kaisalmen commented 10 months ago

Latest status is available: https://www.npmjs.com/package/monaco-editor-wrapper/v/3.4.0-next.5 https://www.npmjs.com/package/@typefox/monaco-editor-react/v/2.4.0-next.5

kaisalmen commented 9 months ago

Thank you @montymxb I merged it after performing the latest changes. https://www.npmjs.com/package/monaco-editor-wrapper/v/3.4.0-next.7 https://www.npmjs.com/package/@typefox/monaco-editor-react/v/2.4.0-next.7

I want to included the latest @codingame/minaco-vscode-api before releasing new final versions.