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

fix(ui): load monaco-editor as a dependency and not from a third party CDN #5189

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

Masterchen09
Copy link
Contributor

This PR adds the monaco-editor as a dependency and loads the editor from the local directory and not from a third party CDN. This fixes potential issues where the CDN is not reachable (see also #4955) and of course it is better to have all dependencies in one place and not relying on a third party service.

As it is quite hard (to not say impossible?) to import the monaco-editor in a Create React App (there are many issues, comments, etc. which describes the issues with it), the files are copied from the monaco-editor package to the build directory using the CopyWebpackPlugin and are then loaded by the @monaco-editor/react package from there.

The version of the added monaco-editor package is exactly the version which is loaded by the current version of the @monaco-editor/react package, therefore there shouldn't be any differences.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions
Copy link

Unit Test Results (build & test)

382 tests  ±0   382 ✔️ +2   9m 16s ⏱️ +29s
  89 suites ±0       0 💤 ±0 
  89 files   ±0       0  - 2 

Results for commit 41e41e8. ± Comparison against base commit af0e348.

@jjoyce0510
Copy link
Collaborator

This a really interesting fix. Totally agree that fetching this from a CDN dynamically was not the solution we wanted.

Let me do a bit of playing around with the changes, assuming all goes well we should be able to get this in!

@jjoyce0510 jjoyce0510 merged commit 6a58398 into datahub-project:master Jun 21, 2022
shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Jun 22, 2022
@Masterchen09 Masterchen09 deleted the fix-monaco-editor branch June 29, 2022 20:26
alexey-kravtsov pushed a commit to infobip/datahub that referenced this pull request Jul 8, 2022
maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022
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 this pull request may close these issues.

2 participants