Skip to content

feat(RendererProvider): add target prop#718

Closed
layershifter wants to merge 1 commit intorobinweser:masterfrom
layershifter:feat/target
Closed

feat(RendererProvider): add target prop#718
layershifter wants to merge 1 commit intorobinweser:masterfrom
layershifter:feat/target

Conversation

@layershifter
Copy link
Contributor

@layershifter layershifter commented Jun 18, 2019

Description

Another approach (from #704) for fixing #693.

RendererProvider

Now this component accepts target prop like in styles-components StyleSheetManager.js#L25, it allows to specify a Document where style nodes will be attached.

subscribeDocument() & documentRefs

This method is added to handle cases with passed documents, this implementation allows us to have a single subscription (difference from #704). subscribeDocument() returns and object with unsubscribe() like subscribe() does.

const r = createRenderer()

r.subscribeDocument(childDocument)
felaDOM.render(r)
// styles will be renderer to main `document` and `childDocument`

However, we need to store Document object somewhere, I propose:

{
  target: Document
  refId: string
  refCount: number
}

We need to have refCount to know when we should remove matching Document and clear renderer.nodes to avoid memory leaks. refId is used by build reference key in renderer.nodes because we can't rely on indexes.

TODO

  • update docs
  • add unit tests

Example

<RendererProvider target={childDocument} />

Packages

List all packages that have been changed.

  • fela-bindings
  • fela-dom
  • fela
  • react-fela

Versioning

Minor

Checklist

Quality Assurance

  • The code was formatted using Prettier (yarn run format)
  • The code has no linting errors (yarn run lint)
  • All tests are passing (yarn run test)
  • There are no flow-type errors (yarn run flow)

Changes

If one of the following checks doesn't make sense due to the type of PR, just check them.

  • Tests have been added/updated
  • Documentation has been added/updated
  • My changes have proper flow-types


import type { DOMRendererDocumentRef } from '../../../flowtypes/DOMRenderer'

const getDocumentRefIndex = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be replaced with .findIndex() but there is no such function in fast-loops and it's not supported by IE11

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.

1 participant