Add key points to Readme. #1218
Conversation
|
Hi Ava, just some general comments. We try to keep commit subjects in the 50-70 character range (full documentation covering all this is coming!). Perhaps you could make the subject Bonus points for the About the changes themselves, if you look closely, you'll notice that we try to keep the text in the README to < 80 character lines (except for things like URLs, commands and output etc). |
README.md
Outdated
| For full details of configuration management, see the | ||
| [docs](https://unit.nginx.org/configuration/#configuration-management). | ||
| ## WebAssembly | ||
| Unit supports deploying WASM as a supported application. |
There was a problem hiding this comment.
- Above we talk about "running apps" but here we're talking about "deploying".
- I'm not sure that calling out WebAssembly is warranted over all the other runtimes. I do like the idea of promoting it but retain a small reservation.
- The abbreviation is Wasm so we should either avoid using two terms or introduce the abbreviation if we intend to use it again.
| Unit supports deploying WASM as a supported application. | |
| Unit supports running WebAssembly (Wasm) Components. |
There was a problem hiding this comment.
About your second point:
I did deliberately call out WebAssembly where other languages are not. My rationale is articulated in the merge request description. My goal is not to promote WebAssembly, but to solve a discovery issue that we have.
Coming to the project, I encountered the unit-wasm repo first. I did not know it was deprecated. I was confused as to why it wasn't mentioned in our Readme since it is a separate repository. I did not realize we had a more up to date module in the Unit repository until someone told me.
Most people are going to find us through Github before they look at our docs site. I am willing to hazard a bet that people will find unit-wasm before they find our largely undocumented WASI module.
Perhaps a better solution is to update unit-wasm with a deprecation notice that redirects to whatever documentation we do have for our WASI module.
Additionally, I think it might be worth explicitly listing all of the languages we support (and probably separate quick starts for each) in the Readme; lest someone skimming our readme miss the one single line we have saying that we support multiple languages. They would come off thinking we only supported PHP.
Happy to either leave this here, or to go update unit-wasm. Please let me know which is preferrable.
There was a problem hiding this comment.
I am hesitant to explicitly mark the wasm language module as deprecated at the moment as it handles some cases that the new one doesn't. E.g ingress/egress of > 4GiB data... we also don't get the same amount of request information, well, not that I found in the C API anyway, see https://gist.github.com/ac000/906707af37b42cdcc9d3cc4c2715da99#file-000-wasi-http-txt-L21
There was a problem hiding this comment.
Your observations are spot-on.
IMO, the job of the readme is to let people know if this is something they should pay attention to, and if so, to get them up and running very quickly. This PR is compatible with that :)
Deprecating unit-wasm and libunitwasm are equally important to that goal.
There was a problem hiding this comment.
Deprecating unit-wasm and libunitwasm are equally important to that goal.
See above.
I could add a note to the unit-wasm README that points people to wasm-wasi-component if they are happy with its caveats (and yes that includes requiring all the rust stuff, or running in memory constrained environments)...
There was a problem hiding this comment.
About the memory constraints.
When uploading a file and simply writing it to disk...
Yes, this does mean that the wasm-wasi-component application process does end
up consuming quite a bit of RAM. When trying to upload the 2GiB file, I clock
it using about ~4GiB, though this does get freed afterwards.
For comparison, the C language module consumes a steady ~70MiB.
This may be fixable...
There was a problem hiding this comment.
I could add a note to the unit-wasm README that points people to wasm-wasi-component if they are happy with its caveats (and yes that includes requiring all the rust stuff, or running in memory constrained environments)...
I think a note would be excellent. I am happy to add one myself. I don't think it's worth going into too much detail on the cost of running the WASI component in the Readme for the unit-wasm repo. Do we have those documented in our official Unit docs?
There was a problem hiding this comment.
It sounds like these performance issues should be an issue, bug, or some other tracked work we can prioritize though.
avahahn
force-pushed
the
ava-readme-bits
branch
2 times, most recently
from
d648334 to
3af679b
Compare
|
You can add my |
avahahn
force-pushed
the
ava-readme-bits
branch
from
7690174 to
51a6285
Compare
avahahn
force-pushed
the
ava-readme-bits
branch
2 times, most recently
from
dc2683a to
b42e9c7
Compare
* expand on docker instructions * identify API documentation * identify WASM documentation Acked-by: Andrew Clayton <[email protected]> Signed-off-by: Ava Hahn <[email protected]>
* This commit adds a warning to readers to clarify that they should be aware of our different image tags before pulling their image.
avahahn
force-pushed
the
ava-readme-bits
branch
from
b42e9c7 to
5b11f5c
Compare
I noted that our Docker instructions don't provide a complete quickstart, and leaves the user thinking about what additional steps need to be taken to host their application. While this isn't too hard to figure out on it's own I find that just having the instruction there saves time otherwise spent looking at examples and finding/reading the Dockerfile.
Additionally, after stumbling on our deprecated WASM functionality before the supported component; I found it prudent to show our WASM documentation front and center. Hopefully this will spare potential users any confusion they get finding the wrong thing first.
Finally, I wanted to point out the configuration docs in their own section, because I missed the single inline link we have to them the first two times I read through our Readme.