Fill out examples by fitzgen · Pull Request #16 · fitzgen/dodrio

fitzgen · 2019-02-12T15:16:31Z

@alexcrichton want to take a look at the TodoMVC implementation?

If you're interested in some lolsob, also check out 5837d53...

alexcrichton

This was quite a pleasure to read over! All the pieces really are seriously coming together here...

I do think we gotta put some elbow grease into creating nodes before this super-awesome, but I'll settle for just awesome for now

alexcrichton · 2019-02-12T15:31:35Z

Cargo.toml


 [profile.release]
-# Tell `rustc` to optimize for small code size.
+incremental = false


Not for long!

alexcrichton · 2019-02-12T15:32:50Z

examples/todomvc/Cargo.toml

+optional = true
+version = "0.4.6"
+
+[dependencies.serde]


I might recommend collapsing these using inline tables where possible (as it's a bit more idiomatic I think)

I updated my cargo update but it still is canonicalizing everything like this :(

Yeah I don't think they're using something which preserves formatting (not sure if that exists in a production-ready capacity even...)

alexcrichton · 2019-02-12T15:33:27Z

examples/todomvc/Cargo.toml

+
+[dependencies.log]
+optional = true
+version = "0.4.6"


You might be able to expose some features of this to compile out debug! and/or info! statements perhaps?

This crate itself doesn't do any logging, just dodrio but it does need to depend on log to get the Level when initializing the console_log logger. So I think it is OK to tie them together in the logging feature since they are only ever used together.

Oh for sure, but debug!'s code is emitted by default, which means there's a runtime check and the code for formatting is all compiled in. For the tiniest binaries we probably want to compile out the log messages?

I've been seeing some debug!s get compiled away when there is no logger -- should double check that for dodrio for sure though.

Oh right that's a good point. I think LTO is good enough to see that if no one sets the max log level the max log level is a constant that forbids logging so everything is optimized away as if it were already compiled out

alexcrichton · 2019-02-12T15:34:16Z

examples/todomvc/index.html

+        <meta charset="utf-8">
+        <meta name="viewport" content="width=device-width, initial-scale=1">
+        <title>dodrio • TodoMVC</title>
+        <link rel="stylesheet" href="node_modules/todomvc-common/base.css">


Is there perhaps an NPM CDN thing we could use to skip the npm install step for this?

Good call: https://unpkg.com/

alexcrichton · 2019-02-12T15:34:54Z

examples/todomvc/src/controller.rs

+
+/// Invoke `f` with the root `Todos` component, save the (likely just modified)
+/// todos to local storage, and schedule a new `dodrio` render.
+fn with_todos<F>(root: &mut dyn RootRender, vdom: VdomWeak, f: F)


Using RAII may make this a bit more ergonomic to call by skipping some indentation maybe?

Not more ergonomic to write initially though

alexcrichton · 2019-02-12T15:38:06Z

examples/todomvc/src/router.rs

+/// Start the router.
+pub fn start(vdom: VdomWeak) {
+    // Callback fired whenever the URL's hash fragment changes. Keeps the root
+    // TODOs collection's visibility in sync with the `#` fragment.


now this is tricky

It's a comment that starts with "TODO"

not a TODO comment

alexcrichton · 2019-02-12T15:39:06Z

examples/todomvc/src/router.rs

+    // Call it once to handle the initial `#` fragment.
+    on_hash_change();
+
+    // Now listen for hash changes forever.


Perhaps a small comment could be added about how this technically isn't linking memory within the context of this one application?

alexcrichton · 2019-02-12T15:39:44Z

examples/todomvc/src/todo.rs

+    }
+
+    /// Get this TODO's title.
+    pub fn title(&self) -> &str {


I smell a derive macro for stuff like this

or just make it all pub... a real application would probably not use pub so I didn't here either. This is kind of an in-between scenario.

alexcrichton · 2019-02-12T15:40:35Z

examples/todomvc/src/todo.rs

+        let title = self.edits.as_ref().unwrap_or(&self.title);
+        let elem_id = bumpalo::format!(in bump, "todo-{}", id);
+
+        let mut input_attrs = bumpalo::vec![


Just gotta say. This, ergonomically, is pretty awesome

alexcrichton · 2019-02-12T15:41:13Z

examples/todomvc/src/todo.rs

+        }
+        let input_attrs = input_attrs.into_bump_slice();
+
+        Node::element(


Heh we should definitely clean this up a bit at some point :)

(the helper methods you were mentioning would be perfect)

(well, that plus maybe a builder to add attributes)

alexcrichton · 2019-02-12T15:43:59Z

Also I demand validation-via-travis.

And I request publication-via-travis.

We can skip an `npm install` now.

Instead of a `with<F>`-style function. It is a bit more idiomatic, and provides nicer usage, but takes a little more to write.

…argo.toml`s

So they don't look like things we need to fix...

fitzgen added 8 commits February 12, 2019 07:02

Add READMEs and comments for examples

307c8d1

Move Render to a private submodule

daa23fe

Deny missing docs and missing Debug implementations

197915d

Only build examples as cdylib

25dcb1c

Fix a bug where "volatile" attributes weren't getting properly updated

5837d53

Add an example TodoMVC implementation

0ac0e9d

Build with LTO

96fcf33

Update the lockfile

e967d2e

alexcrichton approved these changes Feb 12, 2019

View reviewed changes

fitzgen added 9 commits February 12, 2019 10:10

Bump bumpalo dependency (ha)

91b7b60

Use inline tables for dependencies in TodoMVC's Cargo.toml

9f533a4

Use unpkg to get TodoMVC's CSS

13de5e6

We can skip an `npm install` now.

TodoMVC: Use an RAII type to auto render and save to local storage

24743b9

Instead of a `with<F>`-style function. It is a bit more idiomatic, and provides nicer usage, but takes a little more to write.

Make sure we specify the latest version of all dependencies in our `C…

2271ad4

…argo.toml`s

TodoMVC: use ? instead of unwrapping in run start function

1e3cf1f

Use 'todo' instead of 'TODO' for comments in TodoMVC

320d7d9

So they don't look like things we need to fix...

TodoMVC: add small note about why we forget in the router

df5dadf

Fix doc tests

b179f91

fitzgen merged commit ca435e8 into master Feb 12, 2019

fitzgen deleted the fill-out-examples branch February 12, 2019 21:06

Conversation

fitzgen commented Feb 12, 2019

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Feb 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants