-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add Link view, update JavaScriptKit to 0.8.0 #276
Conversation
Fabulous! I was thinking of proposing something like this literally a few days ago, so glad this will be resolved soon 👏 |
Package.swift
Outdated
@@ -4,6 +4,16 @@ | |||
|
|||
import PackageDescription | |||
|
|||
#if os(WASI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually have the desired effect? Package.swift
is evaluated on the host platform, not on the target platform, so #if os(WASI)
condition will never be evaluated until we have a self-hosted toolchain running in the browser (we probably won't for long time). These kind of exclusion of dependencies should be implemented with condition: .when(platforms: [.wasi])
, as done for the TokamakShim
target below.
This check also seems to be the reason for the Linux build failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! 😂
Although conditions don't seem to work, as we can't depend on a version # of JavaScriptKit since it has unsafe build flags, unless I'm doing something wrong? Should I just change the JavaScriptKit dependency to a commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using a recent 5.3 SwiftWasm snapshot as recorded in the .swift-version
file in main
? These snapshots allow adding JSKit with a version as a dependency, even with unsafe flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, Swift toolchains for other platforms don't have the unsafe flags workaround. I hope that if you don't attempt to build products that depend on JavaScriptKit, it won't complain about unsafe flags? As in swift build --product TokamakStaticHTML
, or when you build something that depends on Tokamak, but doesn't depend on TokamakDOM
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my publish test project it seems to resolve the dependencies for all the products, not just TokamakStaticHTML...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we add new LinkerSetting
values, we still need upstream to support them. Now is the best time to submit a PR with these settings upstream before the new major version is branched off, but it means that we still can't create a package manifest that declares these new safe linker settings consumable with upstream Swift 5.3. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way we can take to avoid this issue is to separate this repository into a package that depends on JavaScriptKit and a package that doesn't depends on JavaScriptKit until next major version will be released.
There was a problem hiding this comment.
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, Swift 5.3 still downloads the whole tree of dependencies without checking whether they're used or not. This was fixed in swiftlang/swift-package-manager#2749, but it was merged after Swift 5.3 was branched off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure out another idea 🤷 Tokamak users must use wasm toolchain now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A horrible hack would be to patch SwiftPM to check whether it's linking JavaScriptKit and to inject the required linker flags. This probably isn't worse than what we do now, as SwiftPM in 5.3 SwiftWasm snapshots already checks for a package name and allows unsafe flags if it's JavaScriptKit. But submitting a PR upstream to allow more linker flags is a priority then. I don't want to miss the next version cut-off again. 🙏
@MaxDesiatov this builds and runs with the latest |
Yes, I'll do that in the next few days, there are a few more changes I want to land in SwiftPM to allow OpenCombine tests to run. If it's urgent for you, please feel free to submit a PR to the |
No rush 🙃 |
Sorry for the delay, I'm aware that we have too many stalled PRs and I want to get it all moving again. It's just toolchain and I also hope I'm not completely blocking everything. If there's anything you can't do without me, but you'd like to proceed on your own anyway, please feel free to ping me. I want our projects to be as distributed as possible without bottlenecks in communication and other dev processes. |
Another thing I had in mind for this PR: does this Publish integration require access to anything private or internal in Tokamak? Would it work better as a separate package, and a Publish plugin? |
It can definitely work as a separate package. As for making it a Publish plugin, those aren't really meant for building the theme from my understanding, so I don't think it'd work as one. |
Cool, feel free to create a repo for the package in this org if you'd like. I think a separate repo would be more suitable. It would allow specifying a dependency on a specific version range of Publish. It would also mean we can add support for other static generators in the future separately. Otherwise we'd need to maintain all other integrations within this repo, or Publish would be the only privileged one. |
Would |
LGTM 👍 |
Keeping this PR open because it adds the |
I've uploaded new |
Awesome, CI is now passing 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@carson-katri is this ready to merge? |
Requires #276 to be merged, as earlier versions of JavaScriptKit can't be depended on in macOS builds due to unsafe flags. * Add Link View * Add Publish support * Remove #if checks * Upgrade swift snapshot * Try swiftwasm-action@main * Remove Publish support from this repo * Remove TokamakPublish target * Allow tests to be run on macOS * Update `ci.yml` to build and run the test product * Trigger CI on all PRs without branch restrictions * Rename linux_build to swiftwasm_build in ci.yml Co-authored-by: Carson Katri <carson.katri@gmail.com>
The
Link
View is added in this PR. I also upgraded JavaScriptKit to0.8.0
.This adds the ability to use
TokamakStaticHTML
in place ofPlot
.A
tokamakFoundation
theme is provided, and custom ones can be created with theTokamakHTMLFactory
protocol.That requires implementing the following functions:
These will be rendered to HTML, and embedded in
Plot.HTML
elements.You can then declare your theme with an extension on the
Theme
struct:If you haven't used Publish before, you can install the CLI with:
git clone https://github.com/JohnSundell/Publish.git cd Publish make
Then run
publish new
and addTokamakStaticHTML
as a dependency.