-
Notifications
You must be signed in to change notification settings - Fork 21
Support implementing Symbol-based EcmaScript protocols like Iterable #28
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
base: main
Are you sure you want to change the base?
Conversation
This is most-useful to use NodeSymbol names for methods to implement well-known interfaces, but it could also be useful anywhere you want Swift names and Node names for APIs to diverge. Alternatively, we could add name parameters to the existing `@NodeProperty` and `@NodeMethod` macros. This was more straight-forward to implement for a macro beginner.
- `NodeSymbol.wellKnown(propertyName: "name")` is equivalent to
`Symbol['name']`. This is useful for implementing language-level
protocols.
- `NodeSymbol.global(for: "name")` is equivalent to `Symbol.for('name')` in JS. This is useful for implementing userland protocols.
Both those static methods may throw, so there's also deferred versions
which follow the existing convention of `deferredConstructor` to
postpone building the NodeValue symbol until it's needed on the
@NodeActor thread.
Adds `NodeIterator` class and extension to NodeValueConvertible Sequence to support implementing the EcmaScript iterable protocol from Swift. Adds an iterable protocol integration test to demonstrate how the new features on this branch work together. Discussion point: none of the other internal classes seem to use macros, so I implemented NodeClass manually for NodeIterator. Is it okay to use the macros here?
The dynamic call behavior makes it easy to call random fictional APIs in
Swift code on NodeValue instances. I hit this error several times when
trying to iron out NodeIterator stuff. I was mistakenly doing this:
```swift
try nodeObj.set("prop", newValue)
```
which is legal from the compiler's perspective, but attempts to call a
non-existant function `set` on `nodeObj`. The correct Swift syntax is:
```swift
try nodeObj["prop"].set(to: newValue)
```
By including the DynamicProperty.key in the error message, it's much
easier to find the Swift code responsible for the error throw in
Javascript.
For me, the test harness works fine without calling `builder.clean()` and significantly improves my iteration speed to not have to wait for a full rebuild whenever I run a single test suite. Running all tests should still clean by default, but in both cases this can be configured when running like `CLEAN=1 node index.js suite Test` or `CLEAN=0 node index.js all`.
| final class SomeIterable: NodeClass { | ||
| typealias Element = String | ||
|
|
||
| static let properties: NodeClassPropertyList = [ | ||
| NodeSymbol.iterator: NodeMethod(nodeIterator), | ||
| ] | ||
|
|
||
| static let construct = NodeConstructor(SomeIterable.init(_:)) | ||
| init(_ args: NodeArguments) throws { } | ||
|
|
||
| private let values: [String] = ["one", "two", "three"] | ||
|
|
||
| func nodeIterator() throws -> NodeIterator { | ||
| values.nodeIterator() | ||
| } | ||
|
|
||
| } |
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.
Should this code prefer the macro?
| if let dynamicProperty = self as? NodeObject.DynamicProperty { | ||
| return "\(receiver).\(dynamicProperty.key) is \(actual)" | ||
| } |
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.
Perhaps it would be better for DynamicProperty to implement CustomStringRepresentable in like "(obj).(key)", so that stringifying a DynamicProperty anywhere prints the full path 🤔
|
hey @justjake, thanks for the PR! Just wanted to let you know that I've given the code a first pass and these look like great changes. I'm a tad busy right now but I'll leave feedback asap, hopefully over the coming week. In the meantime, do you mind splitting up the
|
This adds a set of features that allow Swift developers to implement protocols using Symbols, like Iterable:
NodeSymbolto access well-know symbols likeSymbol.iterator, or the global symbol registry likeSymbol.for("user.land.protocol").@NodeName(replacementName)renames members exposed with@NodeClass+@NodePropertyor@NodeMethod. To implement a symbol-based protocol, pass the symbol as the replacementName.NodeIteratorclass to support the Iterable protocol. An example iterable is included in an integration test.Individual commits have some more details.
Discussion points:
@NodeNamebe better as a parameter for the existing@NodeMethod/@NodePropertymacros? It was easier to implement as a new macro for me because I'm new to Swift.NodeIteratorsince I didn't see other uses within the library. Should I convert that class to a@NodeClassmacro? Since there's only one method exposed, it's not much difference in code size in this instance.NodeIteratorclass is used for supporting Swift developers writing Iterable classes to Node, not for consuming Node iterators in Swift, although that could also be useful as well. Should this implementation do both? Or is this kind of thing out of scope for the node-swift library?swift-formatorswiftformattool set up.