Fixes #9.

[Ethorsen]

• Add a mapExtraState option to subspaced enhancer.
• Allow instance enhancers

Q	A
Fixed Issues?	Fixes #9
Documentation only PR
Patch: Bug Fix?
Minor: New Feature?	👍
Major: Breaking Change?
Tests Added + Pass?	No

[mpeyper]

I think we should make a decision one way or the other on these warnings. I'm thinking that this one is ok, as the slice state might legitimately not be a object, but the extraState should always return an object (otherwise how should it be merged?

if (!isPlainObject(extraState)) {
  if (process.env.NODE_ENV !== 'production' && extraState !== null) {
    console.warn(`extra state must be a plain object but received ${extraState}`)
  }
  return componentState
}

I could be convinced that it should throw a TypeError instead of just warn. @jpeyper?

[Ethorsen]

Let me know I'll gladly adapt the PR

[jpeyper]

I'd go with a TypeError (because it is a type error and you've used it wrong... Bang!) in dev and "just work" in production.

[mpeyper]

I think the inclusion in instance enhancers is good, but I'm a little worried that some users may get confused and think that they are able to override the existing enhancers, similarly to have the identifier is replaced and not added to the existing identifier (maybe it should concat them? I'll think about that a bit...).

Any chance of some unit tests for subspaced with a mapExtraState option provided and a createInstance with some extra enhancers?

Can I also ask a quick (mostly) unrelated question. I proposed a change to the API to allow identifiers, and possibly enhancer options as props to the dynamic component instead of having a createInstance function in #10. Do you think that would work for you, or do you like/prefer having the createInstance function?

[Ethorsen]

I think the inclusion in instance enhancers is good, but I'm a little worried that some users may get confused and think that they are able to override the existing enhancers, similarly to have the identifier is replaced and not added to the existing identifier (maybe it should concat them? I'll think about that a bit...).

I was thinking the same, but as they are parameters array I would not know how to de-duplicate/overwrite enhancers of the same type. I also did not want to make a code breaking change.

Any chance of some unit tests for subspaced with a mapExtraState option provided and a createInstance with some extra enhancers?

I'll have a look tomorrow and will try to create and commit tests

Can I also ask a quick (mostly) unrelated question. I proposed a change to the API to allow identifiers, and possibly enhancer options as props to the dynamic component instead of having a createInstance function in #10. Do you think that would work for you, or do you like/prefer having the createInstance function?

I believe that would also work, plus I would not have to keep a reference to the instantiated module.

[Ethorsen]

This is a nasty hack but I haven't found a way to silence react error thrown to console. Even adding and error boundary object did not help.

[mpeyper]

One man's hack is another man's hammer: Exhibit A, B and C

[mpeyper]

I'm not sure that this will ever reset console.error for other tests (possibly hiding useful debugging information)

[Ethorsen]

Exhibit B caught my eye

[mpeyper]

Please also don't forget to add yourself as a contributor

[mpeyper]

These tests should be in the subspaced.test.js file. You can put them in a nested describe('mapExtraState tests', ...) if you want to seperate them from the other tests

[Ethorsen]

Ok

[mpeyper]

Not necessary but you could avoid duplicating the enhancer code by making this

const testEnhancer = instance => identifier => store => {
  expect(identifier).toBe('testId')
  expect(store).toBe(fakeStore)
  callback(instance)
}

and then calling them like

const DynamicComponent = dynamic('baseId', testEnhancer(1))(TestComponent).createInstance('testId', testEnhancer(2))

[Ethorsen]

Sure

[mpeyper]

Was about to merge this then noticed this in my final scan. This is a problematic import for 2 reasons:

1. it is pathing outside of the package into another - this is brittle if the other package ever gets refactored
2. it's referencing the bundled output, which means the other package would need to be built before this test would work - it's purely coincidental that that occurs as part of bootstrapping lerna for us.

in this case, importing it from the core package as you would any other npm dependency is all that is needed. lerna takes care of the rest for us:

import dynostore from '@redux-dynostore/core'

[Ethorsen]

Yeah. Sry I missed it, sometimes IDEA does some weird import when you cut/paste code.

[mpeyper]

Thanks!

As a follow up PR (for you or anyone else), a small example demonstrating a use case for this would be very welcome.

I might wait for #13 before doing a release, but if it takes more than a few days I'll push this out by itself.