-
Notifications
You must be signed in to change notification settings - Fork 68
Add web
OS and architectures
#2108
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
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
I don't believe this is the right solution. We should have a Any code paths building data assets (or other non-code assets) should not have to provide a We most probably didn't refactor the Data assets PR after this change though, so the target OS and target architecture might be carried around in places where they shouldn't be. If we aren't invoking a hook for code assets, we should not have an OS and architecture. Can the Flutter code be refactored to not have to carry around architecture and OS? If not, could you point me to why? |
In the Flutter code, we repeat a build for a list of architectures - so something like
with web having a null architecture, this becomes
I am open to suggestions on how to refactor this any other way. So far, having a |
Future<DartHookResult> _runDartHooks({
required Map<String, String> environmentDefines,
required FlutterNativeAssetsBuildRunner buildRunner,
required List<Architecture> architectures,
required Uri projectUri,
required FileSystem fileSystem,
required OS? targetOS,
required bool linkingEnabled,
required bool codeAssetSupport,
required bool dataAssetSupport,
//TODO(mosum): Remove this parameter as soon as OS and Arch have web values.
required bool isWeb,
}) async {
Can we package all parameters that are for both code assets and data assets in an object (defines, build runner, project uri, file system, linking enabled) and make two methods? Or one method, but have a param Let me just review the whole PR, then I have more context. |
Nice! This would allow the native_toolchain_cmake package to build using Emscripten and add the outputs as data assets for loading via JS. |
Stepping back from that PR, I think there is always a way to make this work. But the question is if adding a @lrhn any opinion or knowledge of prior art? |
Well, adding an |
At least for the near-term, for use with flutter, we would only need the produced wasm and js of a emscripten compiled library moved to the web build folder. This would at least allow us to build data assets. Through the html template, the library can be included and invoked. Perhaps a similar approach could be considered where dart build web provides a html template that includes relevant js/wasm libraries (rather than trying to link it all into a fat library based on if js or wasm), which then frameworks could include where appropriate to make the calls available? |
Opinion: Always. I was planning to ask what it means to be an OS in this context, but even more fundamentally: The doc says The doc also doesn't answer my question. Why does this project need a representation of "operating systems" (whatever that may end up meaning) that Dart runs on? Probably because it needs to make a distinction between them, and (in some cases) treat each distinguishable OS uniquely. The class exists in The class has no members, but has extension members: None of those look relevant to the web. The type (Haven't looked at architecture or how it's used, or intended to be used, but I'd ask the same questions: What problem does adding |
This PR not the right approach. We don't want to add configuration meant for
It has not, so this is indeed the wrong place to add it. I am very open to add support for web assets, but in the Let's continue the design discussion for what config is needed for the web and wasm/js assets on the issues already tracking this:
I believe @mosuem's issue would be solved by flutter/flutter#164094 (comment), please let me know if it doesnt. |
That's a very good point - and why I think we should add the web as an If the issue is about placing those classes only in the |
A note: this is why Flutter does implement As the use of these is quite widespread, perhaps implementing these |
They don't appear in
Data assets may not depend on target architecture. Any other asset types, Wasm, Jar, Js, should have their own config with architecture if they have a concept of architecture.
And we wont support Code assets are meant for dynamic and static libraries that run on a physical hardware architecture. Wasm assets are meant for libraries that run the web assembly hardware abstraction. These are a different asset type.
That should be added as a Let's discuss in: |
I concur with @dcharkes on this one :) There may come a time when we introduce new asset types for web (e.g. JavaScript or Wasm code). Once that time comes we can discuss how to modify the configuration to accommodate this and add e.g. emscripten compiler config, ... For data assets this shouldn't be required atm. (Also flutter's current asset mechanism (afaik) doesn't allow conditionally add icons/fonts/data assets based on architecture or OS, does it?) |
This is an open issue flutter/flutter#96514. After an offline discussion with @dcharkes, I think the bigger picture is that web should not be special-cased as a platform, regardless of whether or not to add it as an OS. For the Flutter PR kicking off this discussion, I found a solution I am quite happy with, abstracting over the target platform instead of using the |
Some data assets may still need to be built based on the target platform, mainly generated content like headers
So this would mean that until this day comes we cannot trigger builds targeting web and would need to piggyback build web libraries or assets while targeting a different OS and include the web assets with all builds.
If the dart build hook controls the conditionals based on architecture or OS, flutter would not need to, it would simply use what is provided. |
Just to clarify, web assets and conditional assets are definitely features we want to and hopefully will ship - this is more of an implementation detail question. |
(Closing as solved differently, and the discussion about implementation details of web code assets is elsewhere) |
See the comment from @bkonyi at flutter/flutter#164094 (comment).
Having to work around the web platform by having a
null
OS
andArchitecture
is awkward, regardless of whether the web is actually an OS or not.For the architecture, I don't really have a use case for separating into
js
andwasm
at the moment, but could imagine this becoming relevant when runningdart compile wasm
vsdart compile js
(vsdart compile exe
on native).Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.