Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'useLeaflet' hook #571

Merged
merged 4 commits into from
May 12, 2019
Merged

'useLeaflet' hook #571

merged 4 commits into from
May 12, 2019

Conversation

vadzim
Copy link
Contributor

@vadzim vadzim commented Feb 19, 2019

Implemented useLeaflet hook.

Example:

const MyComponent = (props) => {
  const { map } = useLeaflet()
  return <>...</>
}

closes #539

@vadzim vadzim changed the title Use leaflet hook 'useLeaflet' hook Feb 19, 2019
@PaulLeCam
Copy link
Owner

Hi,
Do you have a specific use case for this please?

I'd rather not add this kind of support for hooks as an afterthought. The context is meant to be used internally by the components of this library and plugins rather than in a public API, I think it would create confusion about its usage and its limitations.

@vadzim
Copy link
Contributor Author

vadzim commented May 10, 2019

Hi.

Sure. Right now I have 12 files importing withLeaflet in two projects on my job. I'm gonna get rid off HOCs usage in those projects as well as usage of render props like context consumers. The reason is the same why guys have implemented hooks.

As I see my implementation of useLeaflet hook remains leaflet context private.
useLeaflet only shares context value, not the context itself. It lets to read the context value and does not let to write. Exactly the same is done by LeafletConsumer and withLeaflet, but with parallel technologies, with render prop and with HOC respectively.

Could you please expand your understanding of what is wrong with useLeaflet but is ok with LeafletConsumer and withLeaflet?

@vadzim
Copy link
Contributor Author

vadzim commented May 10, 2019

I'd also suggest to use useLeaflet internally in your code to reduce react tree depth:

  const WithLeafletComponent = (props, ref) => (
    <WrappedComponent {...props} leaflet={useLeaflet()} ref={ref} />
  )

instead of

  const WithLeafletComponent = (props, ref) => (
    <LeafletConsumer>
      {(leaflet: LeafletContext) => (
        <WrappedComponent {...props} leaflet={leaflet} ref={ref} />
      )}
    </LeafletConsumer>
  )

@PaulLeCam
Copy link
Owner

I agree hooks are great and could be a better way to implement this library, I've started to experiment with this but I think it should be done in a next major version.

That said, hooks are new and it seems to me people are still figuring out how to use them and/or trying to use them in places they are not the most relevant. More than adding this useLeaflet() hook, I'm concerned about the flow of new issues popping up here saying it "doesn't work" because people try to use this hook without understanding it depends on a parent <Map> context, or calling imperative methods exposed by Leaflet and not having them behave as expected with the rest of the logic.

To me your answer of wanting to replace HOCs with hooks is not really a use case for this library, it's a project setup preference.
Even with this hook, if you wanted to use custom components I suppose you'd extend one of the base components exposed by this library, so really the only case this hook would be relevant would be in the WithLeafletComponent() wrapper, but this is internal logic anyways, it doesn't change the public API.

So to be clear, I agree that hooks are great and useful for this library. It allows a nice functional composition of the logic rather than extending classes for example and I think it's the way forwards, but I don't think adding this useLeaflet() hook with the current architecture of the library provides much benefit for the cost of additional maintenance burden.

@vadzim
Copy link
Contributor Author

vadzim commented May 10, 2019

Sorry to hear that, I really do not understand what is your concern specifically about useLeaflet.

What you're saying can be said exactly about withLeaflet HOC, but HOC is present and hook is not.
So if someone doesn't understand how to use useLeaflet then they don't understand how to use withLeaflet and they can raise the same issues.

Hooks are really not about specific use cases for your library. They are about use cases for programming with react.
Hooks solve some HOC & render prop problems and those problems are common for using all react libraries, not only for using react-leaflet.

I consider that problems, which are solved by hooks, are real and are not the same as project preferences.

@PaulLeCam
Copy link
Owner

PaulLeCam commented May 11, 2019

So what problem does the withLeaflet() HOC create in this library that the useLeaflet() hook solve please?
Any reference to docs or articles mentioning this issue?

@vadzim
Copy link
Contributor Author

vadzim commented May 11, 2019

I'm afraid, I cannot explain better than the react team did.

But theses are the things which annoy me in HOCs:

  • automation tools like eslint can't help if the component stops using some HOC. But eslint will warn if someone forgets to delete the line const { map } = useLeaflet(). Sometimes two PRs removes two different usage of HOC props, and after merging the resulting code contains garbage. Hooks & eslint help in some cases, but HOCs don't.

  • I cannot just use js debugger to move throw the code step by step and to make breakpoints. I should use special HOC to log some info.

  • usually HOCs force to use props with certain names. Sometimes different HOCs can conflict and I'm forced to rename props with special HOC. However this is probably not the case for withLeaflet)

Though afterthought I appreciate your wish to not hurry and take more time thinking on API to have easier maintaining.

@PaulLeCam
Copy link
Owner

OK thanks for the explanations!
I'll tweak your changes to the docs to simply mention the hook as an alternative way to access the context, I hope it will avoid the possible confusion I think might arise from adding this hook.

@PaulLeCam PaulLeCam merged commit 2ad5d85 into PaulLeCam:master May 12, 2019
@vadzim
Copy link
Contributor Author

vadzim commented May 12, 2019

Thanks!

@fas3r
Copy link

fas3r commented Jun 10, 2019

Hello @vadzim ,

I'm sorry to ask but would you mind provide an example ? I'm new in react and a bit lost how to use it.

Thanks

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.

useLeaflet
3 participants