Skip to content

Added option to grab created resource id from original data object.#63

Merged
daffl merged 1 commit into
feathersjs:masterfrom
jayalfredprufrock:jayalfredprufrock-feature/idFromData-option
Sep 14, 2016
Merged

Added option to grab created resource id from original data object.#63
daffl merged 1 commit into
feathersjs:masterfrom
jayalfredprufrock:jayalfredprufrock-feature/idFromData-option

Conversation

@jayalfredprufrock
Copy link
Copy Markdown
Contributor

Maybe somebody can come up with a better option name??

Comment thread src/index.js
return this.db().insert(data, this.id).then(rows => this._get(rows[0], params))
.catch(errorHandler);
return this.db().insert(data, this.id).then(rows => {
const id = this.idFromData ? data[this.id] : rows[0];
Copy link
Copy Markdown
Member

@daffl daffl Aug 3, 2016

Choose a reason for hiding this comment

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

Does it need to be an option flag? I feel we could just fairly safely assume const id = typeof data[this.id] !== 'undefined' ? data[this.id] : rows[0] right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call, an option is totally unnecessary

@daffl
Copy link
Copy Markdown
Member

daffl commented Aug 3, 2016

One comment and a tests for this would be sweet 😄

@jayalfredprufrock
Copy link
Copy Markdown
Contributor Author

Cool I'll whip something up this week

@daffl
Copy link
Copy Markdown
Member

daffl commented Aug 4, 2016

Cool, let me know. I can get to it as well if you are too busy so that we can get this out in a new release.

@daffl
Copy link
Copy Markdown
Member

daffl commented Aug 28, 2016

Any news? I still think it would be a good fix with the changes we discussed.

@daffl daffl merged commit 763bdb1 into feathersjs:master Sep 14, 2016
daffl added a commit that referenced this pull request Sep 14, 2016
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