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

Using internals which may not be visible #738

Closed
jonahgraham opened this issue Jun 1, 2023 · 11 comments · Fixed by #739
Closed

Using internals which may not be visible #738

jonahgraham opened this issue Jun 1, 2023 · 11 comments · Fixed by #739
Milestone

Comments

@jonahgraham
Copy link
Contributor

In MessageTypeAdapter we are using an internal Gson type that is exported in Orbit's gson rebundling (as x-internal) but not in the manifest of the upstream gson.

This forces consumers of jsonrpc (within an OSGi runtime) to use the Orbit version.

@jonahgraham
Copy link
Contributor Author

@cdietrich / @dhuebner looking for a second opinion - is this serious enough that we should do a quick next release? If we (collectively) can fix it immediately then I can do the release ASAP.

@jonahgraham jonahgraham added this to the 0.22.0 milestone Jun 1, 2023
@cdietrich
Copy link
Contributor

cdietrich commented Jun 1, 2023

what i dont understand. why is this not seen in tests. we run non osgi tests
=> is this for persons using maven gson in osgi?

@cdietrich
Copy link
Contributor

i also would prefer a .1 release otherwise we would need a new xtext release

@cdietrich
Copy link
Contributor

cdietrich commented Jun 1, 2023

i am also not aware of people using lsp4j in osgi context
am not sure what about lsp4e

@jonahgraham
Copy link
Contributor Author

what i dont understand. why is this not seen in tests. we run non osgi tests

Without OSGi the LSP4J can access that class (I think? as I don't know how JPMS fits in to the picture, if at all)


On more reflection I don't think this is critical, just annoying as it means LSP4J is forcing LSP4E to use the Orbit gson. We should aim to fix soon, but I don't think we need to respin for this.

@merks
Copy link

merks commented Jun 1, 2023

It’s not critical. Next normal release will be soon enough.

1 similar comment
@merks
Copy link

merks commented Jun 1, 2023

It’s not critical. Next normal release will be soon enough.

@jonahgraham
Copy link
Contributor Author

Thank you @merks for weighing in and giving me that confirmation!

@dhuebner
Copy link
Contributor

dhuebner commented Jun 2, 2023

@jonahgraham
I can prepare a "fix" for this by copying the gson implementation of Primitives.isPrimitive and Primitives.isWrapperType

@dhuebner
Copy link
Contributor

dhuebner commented Jun 2, 2023

@jonahgraham
I've opened a PR #739

@jonahgraham
Copy link
Contributor Author

@jonahgraham I can prepare a "fix" for this by copying the gson implementation of Primitives.isPrimitive and Primitives.isWrapperType

I asked Wayne Beaton review this because I was concerned about the copying but his assessment is in this case it is fine.

@jonahgraham jonahgraham linked a pull request Aug 3, 2023 that will close this issue
jonahgraham added a commit that referenced this issue Aug 16, 2023
Co-authored-by: Dennis Huebner <dennis.huebner@gmail.com>
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.

4 participants