Feature/normalizr/add country entity and students that belongs to contry by nasdan · Pull Request #36 · Lemoncode/redux-by-sample

nasdan · 2017-01-05T16:21:29Z

No description provided.

pablonete · 2017-01-09T09:50:49Z

18 Normalizr/src/model/api/student.ts

    this.gotActiveTraining = false;
    this.fullname = "";
    this.email = "";
+    this.countryId = -1;


-1 smells bad to me. Why not null or undefined?

I think null can be a good value, on the other hand is it a good idea to define this as a class or should it be an interface? Should we add a factory to initialize a new instance?

I think we are using this value because Uncontrolled input warning (If you initialize input value to null, it means that is uncontrolled component) But I think that this initialization has to be component liability and it doesn't delegate to class constructor used in reducer default value state. What do you think?

Yep, if it's a component problem, let the component to deal with it.
On the other hand, I would use an interface too. I avoid factory methods unless you need to share some initialization logic that I don't see on this case.

pablonete · 2017-01-09T09:51:59Z

18 Normalizr/src/model/mappers/countryMapper.ts

+    return {
+      id: country.id,
+      name: country.name
+    }


Does this need a new object? Is not just an interface? In TS, any Country is a CountryView because of structural typing, so just use the original object, even when it has extra props, no?

You are right, we can return original object and in the future, map additional props or some format

pablonete · 2017-01-09T09:52:47Z

18 Normalizr/src/model/mappers/studentMapper.ts

      gotActiveTraining: student.gotActiveTraining,
      fullname: student.fullname,
-      email: student.email
+      email: student.email,


Make trailing comma a convention to avoid this in PRs. It can be set up in TSLint.

pablonete · 2017-01-09T09:54:31Z

18 Normalizr/src/model/mappers/studentMapper.ts

+      email: student.email,
+      country: {
+        id: student.countryId,
+        name: ''


This seems wrong (before looking more code).

pablonete · 2017-01-09T09:57:16Z

18 Normalizr/src/model/view/studentView.ts

    this.gotActiveTraining = false;
    this.fullname = "";
    this.email = "";
+    this.country = new CountryView();


This also seems wrong. Either leave country undefined, or use a CountryView.Empty like with the Null Object pattern. Otherwise it seems you're creating a brand new country for each student.

Agree, or we could assign a default country entry. Thinking now about pros adding a factory to create then StudentView

pablonete · 2017-01-09T10:21:52Z

18 Normalizr/src/reducers/domain/country/index.ts

+export const getCountries = (state) : CountryView[] => {
+  const ids = getIds(state.countryDomain.allIds);
+  return ids.map(id => getCountry(state, id));
+}


Why getCountry and getCountries are on different places?

pablonete · 2017-01-09T10:23:12Z

18 Normalizr/src/reducers/domain/country/list.ts

+  return state;
+}
+
+export const getIds = (state: number[]) => state;


Probably getIds is not needed.

pablonete · 2017-01-09T10:25:52Z

18 Normalizr/src/reducers/domain/student/index.ts

+export const getStudent = (state, id) : StudentView => {
+  return {
+    ...state.studentDomain.byId[id],
+    country: getCountry(state, state.studentDomain.byId[id].country)


I got lost: previously state was the dictionary of students. Now it's the global state. This seems hard to manage unless you strong-type arguments to prevent mismatches.

pablonete · 2017-01-09T10:29:08Z

18 Normalizr/src/schemas/countrySchema.ts

@@ -0,0 +1,4 @@
+import { schema } from 'normalizr';
+
+export const countrySchema = new schema.Entity('countries');


I think we should put all schemas on the same file, so you can export without the schema prefix, like mySchema.country.

Just in case it scales we could go for a solution like

+ src + schema * index.ts * country.ts * student.ts

Then just in index we export all the schemas.

If somebody has to use this he only has to worry about

import "./schema"

But on the name of the schema it self, not sure if we should keep the "Schema" prefix, to avoid creating aliases if there are name collisions.

Yep, the index would be a solution in case we need to break down. Btw I meant suffix, not prefix.

pablonete · 2017-01-09T10:32:32Z

18 Normalizr/src/schemas/studentSchema.ts

+import { countrySchema } from './countrySchema';
+
+export const studentSchema = new schema.Entity('students', {
+  country: countrySchema


This is not true. Current API returns countryId, not a nested object. What I have understood so far is that this line would be helpful to denormalize country information from students, but this is not going to happen. If student gets a country object just for the UI, it doesn't apply here, and probably we would like to stop adding that country to the student object, instead you can look up the country from the Id when needed. Thumbs down to normalizr here, to extract nested object, I see value on it to convert countries array to a dictionary.

brauliodiez · 2017-01-09T11:07:03Z

18 Normalizr/src/reducers/domain/country/index.ts

+  allIds
+});
+
+export const getCountries = (state) : CountryView[] => {


getCountries(state)

This state should be already countryDomain

brauliodiez · 2017-01-09T11:15:37Z

18 Normalizr/src/common/actionsEnums.ts

@@ -6,5 +6,6 @@ export const actionsEnums = {
  STUDENT_FIELD_VALUE_CHANGED: "STUDENT_FIELD_VALUE_CHANGED",


I think we can remove this action is covered by a thunk function

brauliodiez · 2017-01-09T11:17:24Z

18 Normalizr/src/common/components/validationFieldComponent.tsx

+      <div className={wrapperClass}>
+        <label htmlFor={this.props.name}>
+          {this.props.label}
+        </label>


This label should not be included in this component

nasdan added 12 commits January 3, 2017 16:47

Add redux dev tools

ee76103

Add country and countryView

78527b1

Implement country api

79abc3c

Add country, byId and allids reducers

ae14bf5

Fetch countries on load table

67033b5

Update normalizr to 3.0.0 version

acd0450

Fix country field mapping in studentMapper

257bf67

Showing countries in student list

7a8e2cc

Add select element in edit student to edit country

1337a88

Create Select element and added validations

99a4007

Add invalid country custom entry

4e45c7a

Move error property before option property

673f885

nasdan requested a review from brauliodiez January 9, 2017 08:34

pablonete reviewed Jan 9, 2017

View reviewed changes

brauliodiez reviewed Jan 9, 2017

View reviewed changes

nasdan added 11 commits January 9, 2017 18:20

Extract label outside ValidationFieldComponent

d74d20c

return original country in country mapper

a0ba826

Fix typos and dead code

fa7e81a

Rename to getCountryName

1cfac97

Add action to add country

2d884fc

Add index file to schemas

0a1368a

Add custom country from detail component with an action

9aff8ee

Install reselect

47952c6

Add domain selectors

4f86276

Extract student byId reducer into a separated file

9a1be3b

Using typed state

10249d6

		@@ -0,0 +1,4 @@
		import { schema } from 'normalizr';

		export const countrySchema = new schema.Entity('countries');

		@@ -6,5 +6,6 @@ export const actionsEnums = {
		STUDENT_FIELD_VALUE_CHANGED: "STUDENT_FIELD_VALUE_CHANGED",

Conversation

nasdan commented Jan 5, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants