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

feat: codegen based on platform #395

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

patrickkabwe
Copy link
Contributor

Currently, nitrogen generates code for all platforms (code/hybrid objects) regardless of the platform specified in <{platform:lang}> generic tags. This PR modifies the code generation to respect these platform tags and only generate the necessary code for the specified platforms.

For example:

  • <{ios:'swift'|'c++'}> will only generate iOS-specific and shared code
  • <{android:'kotlin'|'c++'}> will only generate Android-specific and shared code
  • <{ios:'swift'|'c++', android:'kotlin'|'c++'}> will generate Android, iOS and shared code

This change improves efficiency by preventing unnecessary code generation for unused platforms.

…ng}>` specified in when creating a hybrid object

Currently, nitrogen generates code for all platforms (code/hybrid objects) regardless of the platform specified in `<{platform:lang}>` generic tags. This PR modifies the code generation to respect these platform tags and only generate the necessary code for the specified platforms.

For example:
- `<{ios:'swift'|'c++'}>` will only generate iOS-specific code
- `<{android:'kotlin'|'c++'}>` will only generate Android-specific code

This change improves efficiency by preventing unnecessary code generation for unused platforms.
Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nitro-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 8:10pm

@mrousavy
Copy link
Owner

mrousavy commented Dec 5, 2024

Hmm, not sure if we want to increase complexity here.

The autolinking setup code is empty anyways if you dont have any Hybrids - not sure if we want to fully remove that code then?

@patrickkabwe
Copy link
Contributor Author

The autolinking setup code is empty anyways if you dont have any Hybrids - not sure if we want to fully remove that code then?

I see your point about empty configs, but I think generating only platform-specific code would be beneficial:

  • Less Clutter: Project files would only contain code for platforms you're actually using
  • Better DX: Makes it immediately clear which platforms are implemented

The implementation complexity would be a one-time cost, but could save confusion down the line. What are your thoughts?

@@ -101,7 +103,8 @@ export async function runNitrogen({
continue
}

const platforms = Object.keys(platformSpec) as Platform[]
platforms = Object.keys(platformSpec) as Platform[]
Copy link
Owner

Choose a reason for hiding this comment

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

This overwrites platforms each time instead of appending to it.

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of lifting out platforms here, let's create a new value in line 82 called usedPlatforms and we additionally append to it.

Copy link
Contributor Author

@patrickkabwe patrickkabwe Dec 5, 2024

Choose a reason for hiding this comment

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

addressed the comment above

@patrickkabwe
Copy link
Contributor Author

Hey @mrousavy, anything else you'd like me to check or tweak on this PR?

@mrousavy
Copy link
Owner

Hey - I'm still thinking about this.

The main reason why I don't want to merge this is because it is not tested in the example here.

There is no module that's iOS or Android only.
I'm afraid that there will be some code in the future that assumes iOS/Android files are there, and it fails on libraries that are iOS/Android only because we didn't test it here in the example.

@patrickkabwe
Copy link
Contributor Author

Hey - I'm still thinking about this.

The main reason why I don't want to merge this is because it is not tested in the example here.

There is no module that's iOS or Android only. I'm afraid that there will be some code in the future that assumes iOS/Android files are there, and it fails on libraries that are iOS/Android only because we didn't test it here in the example.

Alright thanks for the feedback. i will add some tests for this over the weekend.

@mrousavy mrousavy merged commit fe96d24 into mrousavy:main Dec 12, 2024
6 checks passed
@mrousavy
Copy link
Owner

dont worry, lets just use it as is.

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