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

Add Image implementation, bump JSKit to 0.9.0 #155

Merged
merged 11 commits into from
Dec 4, 2020
Merged

Add Image implementation, bump JSKit to 0.9.0 #155

merged 11 commits into from
Dec 4, 2020

Conversation

j-f1
Copy link
Member

@j-f1 j-f1 commented Jul 3, 2020

Note that all images must be specified explicitly in a manifest provided as a tokamakDOM_provideImages modifier. I tried adding the modifier when passing the view to DOMRenderer but it didn’t work until I wrapped it in a view itself (is this a bug?)

Since carton doesn’t support serving local static files, the demo requires an HTTP server running on port 5000 that serves static files from the docs directory.

@j-f1 j-f1 added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jul 3, 2020
@ie-ahm-robox

This comment has been minimized.

@MaxDesiatov
Copy link
Collaborator

I think that both carton and Tokamak should support SwiftPM resources, the only obvious problem being that the Bundle type is declared in Foundation...

@j-f1
Copy link
Member Author

j-f1 commented Jul 3, 2020

Could carton or something else implement the Bundle class itself?

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Jul 3, 2020

It probably could, but I'm not sure whether SwiftPM assumes a specific Bundle implementation in its code and whether we could swap that with our own. Needs some testing and research... 🧪

@j-f1 j-f1 marked this pull request as draft July 5, 2020 17:17
@MaxDesiatov MaxDesiatov changed the title Image Add Image implementation Jul 7, 2020
@MaxDesiatov MaxDesiatov changed the title Add Image implementation Add Image implementation Jul 22, 2020
@j-f1
Copy link
Member Author

j-f1 commented Sep 24, 2020

@MaxDesiatov is there a way to install the latest version of carton from GitHub? The released version doesn’t have resources yet.

@MaxDesiatov
Copy link
Collaborator

Yeah, you can clone the carton repository and just run swift build or swift build -c release in it. Then you can copy the executable from .build/debug/carton or .build/release/carton to any of your PATH directories. brew installs it to /usr/local/bin/carton if you'd like to replace the released version with your own version.

I hesitate to tag a new version because of a regression swiftwasm/swift#1824. OTOH, it's already broken in the current released version that ships 5.3 snapshots by default too. I hope to get it fixed soon, and failing that, will probably release it as is. Maybe I could release it after the next version of JSKit is tagged, so that it's compatible with DOMKit 🤔

@MaxDesiatov
Copy link
Collaborator

Had a quick look at this, and according to SwiftUI docs Image looks into Bundle.main if the bundle argument is nil. This means that all images will be looked up at the root path, unless a user passes some explicit bundle. And the only meaningful value in SwiftWasm Foundation other than Bundle.main (with empty path) is auto-generated Bundle.module.

We can't set Bundle.module as a default argument, as it will always assume that's the bundle of Tokamak itself. So I think we'll stick to the upstream Bundle.main behavior. But it's not that simple, one option is to make carton dev serve resources from executable targets at the root path, which could potentially (but highly unlikely) cause naming collisions with files like dev.js. And the other option is to change the path of Bundle.main in SwiftWasm Foundation to be something like /main. But that would be a hardcoded value for all apps, and we'd now need to wait for 5.3.1 to release that. @TokamakUI/core-team WDYT is the best approach here?

MaxDesiatov added a commit to swiftwasm/carton that referenced this pull request Dec 1, 2020
This is required to make the `Image` view work as described in TokamakUI/Tokamak#155 (comment).

* Demangle and print Firefox stacktraces in terminal

* Update the entrypoints archive URL, rename file

* Add missing newline to `ProcessRunner.swift`

* Remove redundant `console.log` call from `dev.js`

* Detect destination env from `User-Agent` header

* Silence linter in tests where it can't be avoided

* Add support for basic testing in browsers

* Add a browser message on finish, shut down server

* Serve main bundle resources from root directory

* Fix archive hashes, bump JSKit in TestApp to 0.9.0

* Add comments to clarify behavior

* Fix typo in doc comment in `Package.swift`
@MaxDesiatov MaxDesiatov marked this pull request as ready for review December 1, 2020 18:14
@MaxDesiatov MaxDesiatov changed the title Add Image implementation Add Image implementation, bump JSKit to 0.9.0 Dec 1, 2020
@MaxDesiatov MaxDesiatov requested a review from a team December 1, 2020 18:17
@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Dec 1, 2020

@TokamakUI/core-team Now that swiftwasm/carton#176 is merged, this works with images supplied as SwiftPM resources. It does require specifying extensions with image names, and I don't think there's a clean way for us to infer extensions. I've removed systemName initializer as I'm not sure how to proceed with that one right now. We could support some icon sets in a future PR if there's a good way to do it.

Ready for review 🙂


import Foundation

public struct Image: View {
Copy link
Member

Choose a reason for hiding this comment

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

I think to match some of SwiftUI's Image modifiers we'll need to implement the internal storage as an AnyTokenBox. Then once we add support for things like template images we can access the environment to render them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for highlighting this, I've created #312 to track it.

@MaxDesiatov MaxDesiatov requested a review from a team December 2, 2020 09:56
@MaxDesiatov MaxDesiatov merged commit 797c0d8 into main Dec 4, 2020
@MaxDesiatov MaxDesiatov deleted the image branch December 4, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

4 participants