localStorage persistance #10

Martodox · 2017-04-05T19:04:38Z

I had to abstract Todo class away from main file and added a service to delegate the responsibility.

rwjblue · 2017-04-05T19:13:39Z

src/ui/components/services/storage.ts

@@ -0,0 +1,21 @@
+import Todo from '../models/todo';


I believe this should be in either src/ui/components/todo-item/-services/storage.ts (if it is "private" to this component).

Ahh, sorry about this. We cannot nest services under components. I should have said src/services/storage.ts.

Run into next issue
Error: The type of module 'services/storage' could not be identified

Is there a way to do imports from the root of the project, like we have in ember?

import StorageService from 'todomvc-demo/services/storage'

Or something similar?

Ugh, looks like https://github.com/glimmerjs/glimmer-application-pipeline/blob/master/lib/broccoli/default-module-configuration.ts does not define services as a type. We should chat with @dgeb to see if that was intentional, but for now you can put it in src/utils/storage.ts.

(hoping third time is a charm here 😝 )

yea, that bit of code I was missing.

We could override that in config/environment.js, couldn't we?

rwjblue · 2017-04-05T19:14:14Z

src/ui/components/models/todo.ts

@@ -0,0 +1,26 @@
+import { tracked } from "@glimmer/component";


This would generally go in src/data/models/todo.ts or src/ui/components/todo-item/-utils/todo.ts (I personally prefer the former)...

Martodox · 2017-04-05T19:22:27Z

@rwjblue I tried to move the files around, but cli throws this error:
Error: The collection 'components' is not configured to contain a collection 'services'
Any advise?

Martodox · 2017-04-05T20:04:00Z

src/ui/components/todomvc-app/component.ts

@@ -1,30 +1,26 @@
 import Component, { tracked } from "@glimmer/component";
 import Navigo from 'navigo';
+import StorageService from '../../../utils/storage';


@rwjblue Can we do something about those import paths?

Is it possible to use a fully qualified path?

fully qualified path like what?
All I could find based on my research was this reference microsoft/TypeScript#5039 with baseUrl prop on tsconf.json

mmun · 2017-04-05T23:24:12Z

src/ui/components/-utils/todo.ts

+import { tracked } from "@glimmer/component";
+
+export default class Todo {
+  id: number;


The id should be private. It's just a temporary hack because we don't have a good alternative to key="id" at the moment.

mmun · 2017-04-05T23:30:07Z

src/utils/storage.ts

@@ -0,0 +1,21 @@
+import Todo from '../ui/components/-utils/todo';


We shouldn't be importing private modules. I'd prefer to make the Todo class public like the store.

mmun · 2017-04-05T23:30:43Z

src/ui/components/-utils/todo.ts

+    this.completed = !this.completed;
+  }
+
+  serialize() {


I find it a bit confusing that the deserialization logic is in the store, but the serialization logic is in the model. I think the serialization belongs in the store too.

Makes sense, but conflicts with Todo.id being private.

mmun · 2017-04-05T23:32:06Z

src/ui/components/todomvc-app/component.ts

@@ -73,29 +69,35 @@ export default class TodoMVCApp extends Component {
  createTodo(title) {
    this.todos.push(new Todo(title));
    this.todos = this.todos;
+    this.storage.store(this.todos);


All the pairs of

this.todos = ...; this.storage.store(this.todos);

should be extracted to a single method e.g. setTodos.

mmun · 2017-04-06T05:24:36Z

src/utils/storage.ts

@@ -0,0 +1,21 @@
+import Todo from '../ui/components/-utils/todo';
+
+export default class StorageService {


I suggest a more evocative name like TodoStore and the file to todo-store.ts.

mmun · 2017-04-06T05:25:53Z

src/utils/storage.ts

+
+export default class StorageService {
+
+  private storageKey = 'todos-glimmer';


I realize this is a major nitpick but can you change this to 'glimmer-todomvc:todos'?

It comes from todomvc specs todos-frameworkName

mmun · 2017-04-06T05:26:28Z

@Martodox Sorry for the tedious review. I'm happy to make these changes myself if you don't have the time! Thanks for contributing! :)

mmun · 2017-04-06T07:29:52Z

Thanks. I will follow up with some code style tweaks later. Nothing personal ;)

Martodox · 2017-04-06T07:34:42Z

No problem. Happy to help to workout code guidelines for glimmer apps :)

rwjblue reviewed Apr 5, 2017

View reviewed changes

Martodox force-pushed the todo-persistence branch from fec6428 to df7dabd Compare April 5, 2017 20:03

Martodox commented Apr 5, 2017

View reviewed changes

mmun reviewed Apr 5, 2017

View reviewed changes

mmun mentioned this pull request Apr 6, 2017

Quest: Pass TodoMVC spec #3

Closed

22 tasks

mmun reviewed Apr 6, 2017

View reviewed changes

localStorage persistance

148b453

Martodox force-pushed the todo-persistence branch from df7dabd to 148b453 Compare April 6, 2017 07:02

mmun merged commit d807ebf into glimmerjs:master Apr 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

localStorage persistance #10

localStorage persistance #10

Martodox commented Apr 5, 2017 •

edited

Loading

rwjblue Apr 5, 2017

rwjblue Apr 5, 2017

Martodox Apr 5, 2017 •

edited

Loading

rwjblue Apr 5, 2017

rwjblue Apr 5, 2017

Martodox Apr 5, 2017

rwjblue Apr 5, 2017

Martodox commented Apr 5, 2017

Martodox Apr 5, 2017

mmun Apr 6, 2017

Martodox Apr 6, 2017

mmun Apr 5, 2017

mmun Apr 5, 2017 •

edited

Loading

mmun Apr 5, 2017 •

edited

Loading

Martodox Apr 6, 2017

mmun Apr 5, 2017 •

edited

Loading

mmun Apr 6, 2017

mmun Apr 6, 2017

Martodox Apr 6, 2017

mmun commented Apr 6, 2017

mmun commented Apr 6, 2017

Martodox commented Apr 6, 2017

		@@ -0,0 +1,26 @@
		import { tracked } from "@glimmer/component";

		@@ -0,0 +1,21 @@
		import Todo from '../ui/components/-utils/todo';

		@@ -0,0 +1,21 @@
		import Todo from '../ui/components/-utils/todo';

		export default class StorageService {


		export default class StorageService {

		private storageKey = 'todos-glimmer';

localStorage persistance #10

localStorage persistance #10

Conversation

Martodox commented Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martodox Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martodox commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmun Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

mmun Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmun Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmun commented Apr 6, 2017

mmun commented Apr 6, 2017

Martodox commented Apr 6, 2017

Martodox commented Apr 5, 2017 •

edited

Loading

Martodox Apr 5, 2017 •

edited

Loading

mmun Apr 5, 2017 •

edited

Loading

mmun Apr 5, 2017 •

edited

Loading

mmun Apr 5, 2017 •

edited

Loading