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

GLSP-1320: Update to latest glsp dev config #334

Merged
merged 2 commits into from
Apr 22, 2024
Merged

GLSP-1320: Update to latest glsp dev config #334

merged 2 commits into from
Apr 22, 2024

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Apr 17, 2024

What it does

Part of eclipse-glsp/glsp#1320
Fixes eclipse-glsp/glsp#1286

How to test

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

@tortmayr tortmayr force-pushed the glsp-1320 branch 2 times, most recently from 149b758 to 955cb87 Compare April 17, 2024 16:16
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Change looks good to me, thank you Tobias! However, we should definitely avoid to re-export the test-utils again as it creates undesired dependencies.

packages/protocol/src/utils/index.ts Outdated Show resolved Hide resolved
packages/protocol/package.json Show resolved Hide resolved
@tortmayr tortmayr force-pushed the glsp-1320 branch 3 times, most recently from 4cb817a to 866053e Compare April 21, 2024 12:17
- Updates to latest version of @eclipse-glsp/dev
- Add generate:index utility script
- Regenerate index files
- Fix codeActionsOnSave in vscode settings
- Refactor upgrade:next script
- Reformat code base with prettier
- Fix copyright headers (by running a full check with glsp checkHeaders . -t full)
- Add resolutions for snabbdom to fix eclipse-sprotty/sprotty#429
  Can be removed once eclipse-glsp/glsp#1253 is resolved


Part of eclipse-glsp/glsp#1320
Fixes eclipse-glsp/glsp#1286
@tortmayr
Copy link
Contributor Author

tortmayr commented Apr 21, 2024

Thanks for the review @martin-fleck-at
I have resolved your review comments and fixed the failing test step.
Also all changes have been squashed into one commit

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Thank you Tobias, all looks good to me now. I do have a minor request to add some comments above the ignored lines because it is not very obvious why we are doing it. I'd also be interested to know why we need the resolution to snabbdom now.

Comment on lines 1 to 2
utils/test-util.ts
di
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could you please add some comments above each line as to why we are doing this? I don't think it is very obvious, e.g.,:

Suggested change
utils/test-util.ts
di
# do not export test-util in index to avoid a necessary dependency on test frameworks such as chai
# adopters can still use the functionality by importing it through the complete path
utils/test-util.ts
# do not export the DI utilities to avoid a necessary dependency on reflect-metadata
di

"watch": "concurrently --kill-others -n tsc,standalone -c red,yellow \"tsc -b -w --preserveWatchOutput\" \"yarn -s standalone watch:bundle\""
},
"resolutions": {
"snabbdom": "~3.5.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason we need to do this now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I should have read the updated PR description. All good then ;-)

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

LGTM!

@tortmayr tortmayr merged commit 1b6e6f3 into master Apr 22, 2024
6 checks passed
@tortmayr tortmayr deleted the glsp-1320 branch April 22, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inversify is a required dependency for non-webview extension code
2 participants