Skip to content

Control automatic port forwarding with a devcontainer.json file #49

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 27, 2025
Merged

Control automatic port forwarding with a devcontainer.json file #49

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
12f38a1
Control automatic port forwarding with a devcontainer.json file
aaronlehmann Feb 21, 2025
ae1df2f
Validate single port or range endpoint is between 0 and 65535
aaronlehmann Feb 25, 2025
933e1d4
Split devcontainer.json file path out to a settings module
aaronlehmann Feb 25, 2025
cf25cbe
Add validation that onAutoForward is a string
aaronlehmann Feb 25, 2025
69432d7
PortMatcher code review feedback
aaronlehmann Feb 26, 2025
3433d28
Use kotlinx.serialization for JSON deserialization
aaronlehmann Feb 26, 2025
2da0908
Add comment about switching kotlinx-serialization-json to compileOnly
aaronlehmann Feb 26, 2025
8d86ee7
Merge branch 'main' into ports-from-devcontainer-json
bcpeinhardt Feb 27, 2025
fa0df63
changelog
bcpeinhardt Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add comment about switching kotlinx-serialization-json to compileOnly
  • Loading branch information
@aaronlehmann
aaronlehmann committed Feb 26, 2025
commit 2da09089621828cfb6a5d47fee6ad6c516ceac96
5 changes: 5 additions & 0 deletions build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ repositories {
// Dependencies are managed with Gradle version catalog - read more: https://docs.gradle.org/current/userguide/platforms.html#sub:version-catalog
dependencies {
// implementation(libs.annotations)
// We need kotlinx-serialization-json >= 1.7.0 to support allowComments, as
// devcontainer.json files often have comments and therefore use
// nonstandard JSON syntax. This dependency should be marked compileOnly
// once the minimum IDE version we support (pluginSinceBuild in
// gradle.properties) is at least 251, which includes version 1.7.2.
implementation("org.jetbrains.kotlinx:kotlinx-serialization-json:1.8.0")
Copy link
Collaborator

@fioan89 fioan89 Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for changing this.

Since IntelliJ provides it at runtime as well this should be compileOnly. Another important thing is that here we need to declare the version that is packaged with minimum supported IntelliJ.
We support IntelliJ 2022.3 which uses kotlinx.serialization:1.4.1 so that's the minimum version we should use, and it should only be changed when the minimum supported version of IntelliJ changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's unfortunate because this version is too old to support allowComments, and devcontainer.json files often include comments (since the reference implementation and VS Code allow them). It's not an issue in my particular case because I'll be stripping comments before injecting this file, but for maximum compatibility it seems like supporting comments would be a good thing.

Could we depend on this newer version and add a comment that the dependency can be changed to compileOnly once the minimum supported IDE version provides kotlinx.serialization >= 1.7.0? It looks like that minimum IDE version will be idea/251.14649.49 (which includes JetBrains/intellij-community@548a3c9).

Copy link
Collaborator

Choose a reason for hiding this comment

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

as far as I know it should be possible to shadow the dependencies but please test it. As a rule of thumb is best not to shadow intellij dependencies to avoid any classloader/memory leak issue.

Copy link

Choose a reason for hiding this comment

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

Some context: any jars packaged with the plugin are not exposed to the rest of the IDE or other plugins, the plugin has its own classloader, so it's a good way to make plugin stable in its sandbox

So if we need this serialization library and there's a compatibility issue, it's better to bundle it.

Copy link
Contributor Author

@aaronlehmann aaronlehmann Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I added the comment about switching to compileOnly in the future. I've been testing with IntelliJ 242.23339.11 and it seems to work fine, but just tried with EAP 251.22821.72 and it works with that version as well.

Let me know if you think it's better or safer to downgrade the dependency and remove allowComments. I'm also fine with that approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the plugin has its own classloader, so it's a good way to make plugin stable in its sandbox

Correct , and intellij platform sdk also allows the plugins to reference classes provided by other plugins in which the classloader for those plugin dependencies will be used. Not really the case here, so I agree with your statement. It should be safe to pack the serialization library.

Let me know if you think it's better or safer to downgrade the dependency and remove allowComments

I agree with @kirillk there is no need for a downgrade for now.

}

Expand Down
Loading