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

update version of Monaco #639

Closed
jeffmcaffer opened this issue Mar 29, 2019 · 10 comments · Fixed by #856
Closed

update version of Monaco #639

jeffmcaffer opened this issue Mar 29, 2019 · 10 comments · Fixed by #856
Assignees
Milestone

Comments

@jeffmcaffer
Copy link
Member

Lower priority but at some point here we should look to update the monaco-editor and react-monaco-editor packages we are using. They are very old. Not sure there are substantial new features but they are likely >1 year old.

need to be careful thought. The integration was not altogether easy with some funky file copying and a helper build function etc. Perhaps in the new versions they have simplified this?

@storrisi
Copy link
Contributor

storrisi commented May 9, 2019

@jeffmcaffer do you remember which kind of problem did you faced during the integration? I'd love to update the version since i'm on the way to made some work about loading large payloads.

@storrisi storrisi added this to the May 2019 milestone May 9, 2019
@storrisi storrisi self-assigned this May 9, 2019
@storrisi
Copy link
Contributor

storrisi commented May 9, 2019

I've tried to update it and i'm not noticing any breaking change.
BTW we have some styling issues compared to what we have noiw that i'm going to fix.

Schermata 2019-05-09 alle 16 59 39
Schermata 2019-05-09 alle 16 59 48

@dabutvin
Copy link
Member

dabutvin commented May 9, 2019

while we are in here can we make sure we get collapsing/folding for the json?
https://microsoft.github.io/monaco-editor/api/interfaces/monaco.editor.ieditorconstructionoptions.html#folding

@storrisi
Copy link
Contributor

storrisi commented May 9, 2019

@dabutvin sure!

@jeffmcaffer
Copy link
Member Author

The were just challenges in getting the deployment working. Had to copy files around at build time etc. The was some fuss around syntax highlighting and content assist.

+1 to @dabutvin suggestion about folding. That would be a huge win.

@storrisi
Copy link
Contributor

@jeffmcaffer for the reason you mentioned i would suggest to eject the application from CRA. Things like this one could be addressed by working on the Webpack configuration rather than applying workarounds.

I hope to have the chance to discuss with you further more on this topic.

@storrisi
Copy link
Contributor

folding property and syntax highlighting could be enabled only by customizing the Webpack plugin configuration:
https://github.com/Microsoft/monaco-editor-webpack-plugin#options

@storrisi
Copy link
Contributor

Following the issue microsoft/monaco-editor#82 there are different approaches proposed.

Someone injects the external Monaco Editor library instead using it as a dependency, but with that approach is not possible to customize options.

Other guys uses https://github.com/timarney/react-app-rewired , or https://github.com/harrysolovay/rescripts , that allows to override part of the Webpack configuration without ejecting, but i'm not so happy about these solutions, they would lost all the benefits of using CRA like an ejection does, and they are not standards.

@jeffmendoza jeffmendoza modified the milestones: May 2019, November 2019 Oct 31, 2019
@tmarble
Copy link
Contributor

tmarble commented Nov 25, 2019

@storrisi I have found this background to be useful in understanding the issues in CRA configuration and "ejecting".

This is one of the alternatives listed for react-app-rewired. Another one which looks promising is craco. Both of these claim to support customizing without resorting to "eject".

So my question to you is: what are the benefits of "eject" that outweigh the solutions of rescripts or craco?

NOTE also this recent approach to using monaco-editor.

And the second question is: would this "recent approach" (using the https://github.com/bitauth/create-react-app-bitauth-ide fork of CRA) be "better" then either of the above (eject and ??, rescripts -or- craco w/o eject)? Realize that you would be hoping that the fork would continue to track upstream CRA (which may be risky).

The answers here should guide the next attempt at upgrading.

@tmarble
Copy link
Contributor

tmarble commented Nov 26, 2019

To document our discussion on Discord today I plan to investigate https://www.npmjs.com/package/@monaco-editor/react

tmarble added a commit that referenced this issue Dec 1, 2019
Replaces accomodations based on react-monaco-editor..
- src/components/MonacoEditorWrapper.js
- src/utils/install-monaco.js

... with @monaco-editor/react
- Updates RawDataRenderer accordingly
- Adds certain new Monaco Editor options
  + cursorSmoothCaretAnimation: true,
  + cursorStyle: 'block',
  + cursorSurroundingLines: 1,
  + mouseWheelZoom: true
  FFI: https://microsoft.github.io/monaco-editor/api/interfaces/monaco.editor.ieditoroptions.html
  NOTE: folding is true by default

Adds explicit dependency on typescript
- As required by transitive deps: tsutils, ts-pnp

Resolves: #639

Signed-off-by: Tom Marble <[email protected]>
@geneh geneh closed this as completed in #856 Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants