gpbl / denormalizr

Denormalize data normalized with normalizr
https://www.npmjs.com/package/denormalizr
MIT License
228 stars 24 forks source link

Support normalizr 3.0 #35

Closed brunolemos closed 7 years ago

brunolemos commented 7 years ago

~~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 commented 7 years ago

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 commented 7 years ago

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

brunolemos commented 7 years ago

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

import { normalize, denormalize } from 'normalizr';
fredefl commented 7 years ago

Doesn't this commit: https://github.com/paularmstrong/normalizr/commit/2522bb91def7f8bae99097f1af462121610188ed

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

coveralls commented 7 years ago

Coverage Status

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

brunolemos commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Coverage Status

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

diegohaz commented 7 years ago

Great work, @brunolemos. Thank you very much.

brunolemos commented 7 years ago

@diegohaz no problem! 🇧🇷

paularmstrong commented 7 years ago

FYI: https://github.com/paularmstrong/normalizr/pull/214

brunolemos commented 7 years ago

@gpbl :shipit:

gpbl commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

paularmstrong commented 7 years ago

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