-
Notifications
You must be signed in to change notification settings - Fork 43
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
feat(isolatedwit): Moving the adder wit to a separate package. #136
feat(isolatedwit): Moving the adder wit to a separate package. #136
Conversation
component-model/src/tutorial.md
Outdated
@@ -133,7 +145,7 @@ Now it all adds up! Run the final component with the `wasmtime` CLI, ensuring yo | |||
the `wasmtime` command line do not include component model support. | |||
|
|||
```sh | |||
wasmtime run final.wasm 1 2 add | |||
wasmtime run command.wasm 1 2 add |
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.
We are still creating final.wasm
on line 134... is that no longer used? Doesn't command.wasm
have unsatisfied imports at this point?
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 meant to overwrite command.wasm. But I missed updating line 134 .
I think it is better to leave it as final.wasm. Will update soon.
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.
Updated . @itowlson
component-model/src/tutorial.md
Outdated
} | ||
eval-expression: func(op: op, x: u32, y: u32) -> u32; | ||
} | ||
//adder/wit/world.wit |
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.
Provide more intro here. This now goes from a paragraph talking about the "calculate" interface/world to a WIT that is just the adder
interface/world. E.g. // WIT for the "adder" world - see <file path>
, or a bridging paragraph.
Or break up the "these files" define section into e..g
The "adder" file defines:
(list of bullets)
... WIT WIT WIT ...
The "calculator" file defines:
(list of bullets)
... WIT WIT WIT ...
(I'm not sure which approach will work best - we may need to experiment a bit!)
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 added description for the adder with separately. please let me know if this works . @itowlson
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.
Yep, that works - thanks @ritesh089!
component-model/src/tutorial.md
Outdated
|
||
world calculator { | ||
export calculate; | ||
import add; | ||
import docs:adder/add; |
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.
This adds a new concept of cross-package references. Do we need to explain that here? It feels like it complicates the tutorial, but as a reader I would have questions given that these world have supposedly come from different sources. Maybe a forward reference is enough?
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 think cross package reference example is very much required. Because the real use cases for most teams to use component model will be to build components independently and compose them later as and when required.
I felt that was missing in the example and documentation. which was the main reason for me to create a separate repo which my team used to understand this concept.
I did not understand the forward reference part .
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.
By "forward reference" I mean a link to a section of the documentation that explains it (rather than explaining it in the tutorial itself).
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.
Based on our discussion, the scope of this PR will not be to update the language reference.
@@ -67,14 +74,14 @@ world app { | |||
|
|||
Reference the [language guide](language-support.md) and [authoring components |
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 the language guides going to need any update to reflect that we now have two WIT packages/files in play?
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 kept the scope of this PR to the tutorial section. The language reference doesn't use this tutorial as a link reference , but I do see some differences.
Do you want me to update that in another PR ? Or you prefer all the changes in one PR here itself?
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.
If the language guide is general guidance and not tied to the tutorial files layout, then there is no need to change it.
@@ -67,14 +74,14 @@ world app { | |||
|
|||
Reference the [language guide](language-support.md) and [authoring components | |||
documentation](creating-and-consuming/authoring.md) to create a component that implements the | |||
`adder` world of `calculator.wit`. For reference, see the completed | |||
`adder` world of `adder/wit/world.wit`. For reference, see the completed | |||
[example](https://github.com/bytecodealliance/component-docs/tree/main/component-model/examples/tutorial/adder/). | |||
|
|||
## Create a `calculator` component | |||
|
|||
Reference the [language guide](language-support.md) and [authoring components |
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.
Again, any updates needed to the language guides?
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 am a bit confused about this.
@kate-goldenring asked me to keep the scope minimal. I am not sure if I should update the language guide.
Can you please confirm?
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.
Per the other thread (#136 (comment)), if the language guide is general guidance and not tied to the tutorial files layout, then there is no need to change it.
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.
ok
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.
@kate-goldenring @itowlson , Did you get a chance to review this PR?
What are the next steps?
Co-authored-by: itowlson <[email protected]>
package = "docs:calculator" | ||
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
|
||
[dependencies] |
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.
Any reason for the reordering here?
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 changed the package name to command to show that these components can be isolated in their own packages. Do you prefer to keep the package "docs:calculator"? @kate-goldenring
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.
This looks okay to me - thanks @ritesh089 for working through all the discussion! Holding for @kate-goldenring who knows the tutorial pages better than I do!
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.
Thank you @ritesh089. I tested everything locally and it looks good. Some small nits to finish cleaning up the WIT paths but LGTM
Co-authored-by: Kate Goldenring <[email protected]>
Co-authored-by: Kate Goldenring <[email protected]>
Co-authored-by: Kate Goldenring <[email protected]>
Co-authored-by: Kate Goldenring <[email protected]>
Co-authored-by: Kate Goldenring <[email protected]>
This PR updates the tutorial example that shows that components can be built with their own wit and then composed together with another parent component.
This PR is based on the feedback provided by @kate-goldenring on
#135