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

Multiple library products and union types #14

Closed
MaxDesiatov opened this issue May 2, 2022 · 11 comments · Fixed by #46
Closed

Multiple library products and union types #14

MaxDesiatov opened this issue May 2, 2022 · 11 comments · Fixed by #46
Labels
enhancement New feature or request

Comments

@MaxDesiatov
Copy link
Contributor

Playing with the library a bit more, I think it would be great to split it into separate targets and products, if possible. As an example, people who don't need WebGL don't need to build WebGL wrappers, but I think it still makes sense to provide them.

IDK if WebIDL specifies dependencies between their "modules", so maybe we'd need to hardcode those on our side for things to work?

@MaxDesiatov MaxDesiatov added the enhancement New feature or request label May 2, 2022
@MaxDesiatov MaxDesiatov changed the title Splitting into multiple library products Split DOMKit into multiple library products May 2, 2022
@j-f1
Copy link
Member

j-f1 commented May 2, 2022

WebIDL doesn't really specify dependencies as far as I can tell, and any spec can define additional properties/methods on a type from any other spec.

@j-f1
Copy link
Member

j-f1 commented May 2, 2022

Ultimately I think we need an overlay with some manual citation of the generated code. For example, canvas.getContext() can return a WebGL context, which requires that we have all the WebGL types included. A better API for Swift would be to have canvas.contextXXX accessors that return a specified context type, replacing the need for getContext and therefore allowing WebGL to be optional.

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented May 2, 2022

I agree.

Not necessarily related to the issue of splitting into separate products, but we also need to do something about ugly nullable_Double_or_seq_of_nullable_Double types. Maybe a better way to handle them would be to create overloaded functions/initializers that take arguments of different types and pass them in to JavaScript directly, avoiding the need for these ad-hoc enums altogether.

@j-f1
Copy link
Member

j-f1 commented May 2, 2022

I think for parameter types we should be able to make those union types into protocols, so that you can pass any value. That does mean the Context type will need to get a new property that controls whether the types it’s printing will be “in” types (i.e. going to JS) or “out” types (i.e. going to Swift) [I have no idea if there are words for this already]. Then we’d be able to pick between having the enum as the return value and having the protocol existential as the parameter.

The ugly names are unfortunate, though. I have some code already that renames them to a human-readable name if they’re used inside a typedefs (i.e. a hypothetical typedef (string | double) ObjectKey would be called ObjectKey instead of String_or_Double). For types that don’t have a good name (since they’re used directly in method types, for example), we could always write custom names and put them in a mapping somewhere.

@MaxDesiatov
Copy link
Contributor Author

Is there a downside to my proposed overloads solution that makes you prefer union types as protocols over overloads?

@j-f1
Copy link
Member

j-f1 commented May 2, 2022

It would make the code generator more complicated (although that’s no justification, I simply means I’m lazy haha). But also due to the way Cartesian products work, we would end up with many same-named overloads if there were methods with multiple union-typed parameters. Finally, is it even possible to overload properties? They can have a union type too.

@MaxDesiatov
Copy link
Contributor Author

This makes sense. BTW, protocols have a runtime performance overhead over enums, but I agree they may be more suitable as they are more flexible in terms of future additions. Enums are quite rigid, as we've already witnessed with JSValue breakage.

CC @kateinoigakukun.

@MaxDesiatov MaxDesiatov changed the title Split DOMKit into multiple library products Multiple library products and union types May 2, 2022
@j-f1
Copy link
Member

j-f1 commented May 2, 2022

Mmm, good point about the enums. Maybe we should make our union type’s concrete implementations be structs, then. Or maybe we don’t need a concrete type at all, just a function that can decode a JSValue to any of the supported types (from my example above:

// maybe an extension on JSValue? Maybe public or maybe internal?
func foo(_ value: JSValue) -> ObjectKey {
  String.construct(from: value) ?? Double.construct(from: value)
}

I wish there were a way to define methods directly on protocol existentials…

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 2, 2022

For the performance overheads, if user passes a concrete type to a function that takes an existential abstracted type and the callee is inlinable, the boxing overheads can be omitted.

Unlike JSValue, library users wouldn't use the union type directly and wouldn't store an object as the union type in properties. Therefore we can assume that the boxing would happen only in passing arguments and output objects, and the former boxing can be omitted by inlining.

@kateinoigakukun
Copy link
Member

kateinoigakukun commented May 2, 2022

This feature is very important in the aspect of build artifact size. Ideally, the size issue should be resolved by cross module DCE, but the latest compiler doesn't do it now, so we need to split modules carefully.

@MaxDesiatov MaxDesiatov linked a pull request Aug 27, 2022 that will close this issue
MaxDesiatov added a commit that referenced this issue Aug 27, 2022
New `Module` type is added that specifies dependencies between modules manually.

More advanced approaches could be implemented in the future. For example, automated dependency resolution as investigated in #30.

Partially resolves #14.
@MaxDesiatov
Copy link
Contributor Author

I'm closing this as multiple library products are now available in main. A more robust approach for union types can be discussed in a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants