Skip to content

Update multiplatform-ktor-sqldelight.md #250

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

Closed
wants to merge 1 commit into from

Conversation

kpgalligan
Copy link

Extended the section on sqlite linker flags. While I understand this is an overview for simplicity, this section is showing up as the "official solution", and there are real reasons you wouldn't want a dynamic framework.

OK if you don't want to complicate the docs, but had to try :)

Extended the section on sqlite linker flags. While I understand this is an overview for simplicity, this section is showing up as the "official solution", and there are real reasons you wouldn't want a dynamic framework.

OK if you don't want to complicate the docs, but had to try :)
@kpgalligan kpgalligan requested a review from a team as a code owner October 2, 2024 14:15

The Kotlin Multiplatform wizard generates projects set up for static linking of iOS frameworks.
To use the SQLDelight library, allow the iOS framework to link dynamically.
The pros and cons of static vs dynamic linking are only important for production apps. To make linking simple for now, link your framework dynamically.

Choose a reason for hiding this comment

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

These docs are a bit out of date for another reason: in recent versions of KMP, dynamic linking is already the default. So it makes no sense to have a section titled "Turn off static linking".

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you consider "recent versions of KMP"? Most of the samples in the docs are based on the KMP wizard, and the latest project generated there still has isStatic = true in that section.

Choose a reason for hiding this comment

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

I'm talking about the default for the Kotlin gradle plugin, not the KMP wizard.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. The section is explicitly written in the context of the tutorial and the wizard, so I don't think there should be any confusion here.

It was pointed out to us that one can remove the isStatic field rather than set it to false since this is the default value, and maybe that's what we should advise here. I think we opted for clarity last time, but I'm not so sure now.

Choose a reason for hiding this comment

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

Ah, given this is written in the context of the wizard which sets it to true, it makes sense.

@zamulla
Copy link
Contributor

zamulla commented Oct 7, 2024

@kpgalligan I started to review this, but I think it's easier on us and the reader just to ask them to add the linker flag. The production considerations sound like a tease without actual deeper dive to refer to :)

I'm closing this PR, but please tell me if I missed something in the one I made: #253

@zamulla zamulla closed this Oct 7, 2024
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.

3 participants