Skip to content
This repository has been archived by the owner on Nov 17, 2017. It is now read-only.

Support normalizr 3.0 #35

Merged
merged 1 commit into from
Feb 11, 2017
Merged

Support normalizr 3.0 #35

merged 1 commit into from
Feb 11, 2017

Conversation

brunolemos
Copy link
Contributor

@brunolemos brunolemos commented Dec 29, 2016

Hi, it's almost done!
28 tests passing, only 2 remaining.

100% working!


While it's not merged, people can use my fork:

package.json:

"denormalizr": "https://github.com/brunolemos/denormalizr",

@paularmstrong
Copy link

paularmstrong commented Dec 29, 2016

I was thinking about supporting "denormalization" as a separate package https://github.com/paularmstrong/denormalizr. It extends normalizr, so it would be used as a drop-in replacement. However, I'm still working on writing "why" I think it's best to keep it separate...

@brunolemos
Copy link
Contributor Author

brunolemos commented Dec 29, 2016

@paularmstrong official denormalizer would be great! please consider supporting Immutablejs as well.

@brunolemos
Copy link
Contributor Author

@paularmstrong they would fit well in the same npm package tbh

import { normalize, denormalize } from 'normalizr';

@fredefl
Copy link

fredefl commented Dec 31, 2016

Doesn't this commit:
paularmstrong/normalizr@2522bb9

Break your pull request @brunolemos? As such, the code provided still uses getKey() instead of .key.

@brunolemos brunolemos force-pushed the normalizr-3 branch 3 times, most recently from 3f968e6 to e87dab5 Compare December 31, 2016 19:29
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e87dab5 on brunolemos:normalizr-3 into 08ede86 on gpbl:master.

@brunolemos
Copy link
Contributor Author

brunolemos commented Dec 31, 2016

Thanks @fredefl, updated the pull request!

And also found the problem with the 2 tests that were remaining.
It's actually the tests that were wrong, but I'll leave it to @gpbl to check it the change I made makes sense.
when defining a relationship should return the original response

100% working now!

@fredefl
Copy link

fredefl commented Dec 31, 2016

You're absolutely welcome @brunolemos!

I am actually already using your normalizr3 fix to denormalizr, and so far it has been rock solid. Thanks a bunch man!

@gpbl
Copy link
Owner

gpbl commented Jan 2, 2017

@brunolemos thanks for your PR and your work 👍

I'm on sabbatical without my laptop until the end of the month, so I'll be slow publishing this. Let me check the changes first :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8d33a7f on brunolemos:normalizr-3 into 08ede86 on gpbl:master.

@diegohaz
Copy link

Great work, @brunolemos. Thank you very much.

@brunolemos
Copy link
Contributor Author

@diegohaz no problem! 🇧🇷

@paularmstrong
Copy link

FYI: paularmstrong/normalizr#214

@brunolemos
Copy link
Contributor Author

@gpbl :shipit:

@gpbl gpbl merged commit c3b4bf3 into gpbl:master Feb 11, 2017
@gpbl
Copy link
Owner

gpbl commented Feb 11, 2017

Sorry for the delay, I was traveling 🗺

This is published now as v0.5.0.

However, I wonder about this package since @paularmstrong has implemented it in the original module 😅

@brunolemos
Copy link
Contributor Author

I was gonna say they don't support immutable by apparently it was added in the last version 😯 https://github.com/paularmstrong/normalizr/releases/tag/v3.2.0

@paularmstrong
Copy link

Speaking of which. Is there anything that this package supports that normalizr doesn't? Is it still helpful to keep two packages around that do the same thing?

@gpbl
Copy link
Owner

gpbl commented Feb 11, 2017

@paularmstrong

Is it still helpful to keep two packages around that do the same thing?

Not in my opinion 😄
I think this module should be deprecated. People were working on nice features however, such as memoization.

@paularmstrong
Copy link

I don't think I'd like to see memoization in normalizr. At Twitter, we use reselect to handle memoization at a higher level.

@gpbl
Copy link
Owner

gpbl commented Feb 11, 2017

Yup I don't see its need either. I believe I'll deprecate the package

@paularmstrong
Copy link

@gpbl awesome! Don't ever hesitate to reach out! You may also want to wait until paularmstrong/normalizr#238 gets in before deprecating.

@gpbl gpbl mentioned this pull request Feb 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants