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

No more ngx-monaco #155

Merged

Conversation

1000TurquoisePogs
Copy link
Member

Update monaco to newest version. Remove need for native compilation. Remove ngx-monaco. Reduce dependency on globals. Update packages to reduce npm audit warnings

Signed-off-by: 1000TurquoisePogs sgrady@rocketsoftware.com

…Remove ngx-monaco. Reduce dependency on globals. Update packages to reduce npm audit warnings

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
@1000TurquoisePogs 1000TurquoisePogs requested a review from a team June 13, 2020 01:02
Copy link
Member

@DivergentEuropeans DivergentEuropeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, other than the small things I mentioned, things look generally OK to me. However, I am getting some very funky visual problems, which leads to me to believe that something is wrong with the way CSS is being imported, using Node 10.16.3, Windows 10, Chrome

Broken highlighting, line numbers not showing, weird artifacts & cursor does not update with the correct location.
image
Functionally, it seems to work OK, Ctrl + A correctly selects all even if visually it doesn't show it, cursor types from the correct spot even if visually it doesn't show it

Syntax highlighting works though

"source-map-loader": "~0.2.4",
"style-loader": "~0.22.1",
"ts-loader": "~4.4.2",
"ts-node": "~4.1.0",
"tslint": "~5.10.0",
"typescript": "~2.7.2",
"typescript": "~3.7.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Angular 9 supports 3.7 I believe, Angular 8 supports 3.4

So don't remember what Angular version we're responsible for (6?), just making sure you have the version you intended it to be in this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not because I wanted to but because monaco itself needs typescript of a certain version. This seems to work even though angular doesnt officially support it. I think it just means "dont do newer typescript features with older angular", which is fine because here, the newer typescript features are being used for monaco, which lives outside of angular.

Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Signed-off-by: 1000TurquoisePogs <sgrady@rocketsoftware.com>
Copy link
Member

@DivergentEuropeans DivergentEuropeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes addressed, I see no regressions and bug has been fixed 👍

@DivergentEuropeans DivergentEuropeans merged commit f73c568 into zowe:staging Jun 17, 2020
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