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

Spruce up the README #25

Merged
merged 5 commits into from
Sep 7, 2022
Merged

Spruce up the README #25

merged 5 commits into from
Sep 7, 2022

Conversation

fosskers
Copy link
Contributor

@fosskers fosskers commented Dec 20, 2021

This PR makes the README a bit easier to read in plain text, and also replaces the large type conversion bullet-lists with charts. This makes it easier to tell what types map to what at a glance.

Note regarding some of the content in Motivation: The WASM anyref proposal seems to have been merged: WebAssembly/reference-types#140 (comment). How does that affect this README (or project)?

@RReverser
Copy link
Owner

Just a heads up - this is fairly significant change, I'll take somewhat longer to get around and review it.

@fosskers
Copy link
Contributor Author

The paragraphs are mostly the same, just refitted to match the usual 80-character width limit. And I moved the URLs out to make the plaintext nicer on the eyes.

Otherwise yes, the Type Tables are totally new.

attribute can define the expected Javascript representation of the variant, or
if [serde-rs/serde#1183] gets resolved.

[safe integer]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I prefer to keep links inline as it makes it easier to edit/remove them without looking for definitions somewhere else in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always keep the links underneath the paragraph they belong to, meaning they're always in one place and thus easy to find, but if you prefer inlined I can revert.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I rarely want links together with each other, but rather with their surrounding contexts, so I'd prefer to revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've inlined all the URLs except two; the ones that appear in the chart above. Otherwise the chart would get quite ugly.


### Deserialization

| Javascript Type | Rust Type |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the table idea overall. I wonder though, if we go there, perhaps it's better to merge both tables into one, with one column for "idiomatic type" (both used for serialization and supported for deserialization) and another column for "other supported sources" (extra types supported for deserialization only).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I'm in favour of that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you planning to come back to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at it again, fusing the charts might be harder than we initially thought. Each serialization direction has information unique to it, as well as caveats, so maybe it's clearer to keep them separate?

@RReverser
Copy link
Owner

Note regarding some of the content in Motivation: The WASM anyref proposal seems to have been merged:

About this - the proposals definitely had lots of forks & renames since that was written. Now it's interface types that in theory should alleviate that cost, but I'd rather just remove that sentence altogether and not promise anything at this point :)

@fosskers
Copy link
Contributor Author

I will return to this soon.

@RReverser RReverser mentioned this pull request Jun 15, 2022
@RReverser RReverser force-pushed the master branch 2 times, most recently from 6c2e781 to 1517453 Compare September 1, 2022 20:34
@RReverser
Copy link
Owner

I'm going to change the main text to what the official wasm-bindgen guide has now that serde-wasm-bindgen is the official approach, but want to keep the table and your credits, so I'll merge this PR first. Thanks for your work!

@RReverser RReverser merged commit 95c935c into RReverser:master Sep 7, 2022
@fosskers
Copy link
Contributor Author

fosskers commented Sep 9, 2022

Thanks!

@fosskers fosskers deleted the colin/readme branch September 9, 2022 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants