elgorditosalsero / react-gtm-hook

Easily manage the Google Tag Manager via Hook
https://elgorditosalsero-react-gtm-hook.netlify.app/
MIT License
220 stars 28 forks source link

Thoughts about lib #17

Closed precious-void closed 3 years ago

precious-void commented 3 years ago

Hey, pals, thanks for lib! I've been using your project to work with GTM and I ran into bugs like twice mounting or cannot push to undefined (https://github.com/elgorditosalsero/react-gtm-hook/issues/16 https://github.com/elgorditosalsero/react-gtm-hook/issues/13) So I was thinking, maybe there is a way to make it better. Firstly, because the init function is called twice sometimes it causes some troubles, so what if we pass the initial state directly into the GTM Provider Params?

    <GTMProvider state={gtmParams}>
        <SomeComponentHere />
    </GTMProvider>

And secondly, why not separate sendGTMData (dispatchGTMEvent) hook and UseGTMHookProvider (GTMProvider)? So we would just initialize it only one time and after that, we would be able to set the data directly through the dispatchGTMEvent hook?

/**
 * Contexts
 */
export const GTMContext = createContext<ISnippetsParams | undefined>(initialState)
export const GTMContextDispatch = createContext<any | undefined>(undefined)

function dataReducer(state: ISnippetsParams, data: any) {
  sendToGTM({ data, dataLayerName: state?.dataLayerName! })
  return state
}

/**
 * The Google Tag Manager Provider
 */
function GTMProvider({ state, children }: GTMHookProviderProps): JSX.Element {
  const [store, dispatch] = useReducer(dataReducer, { ...initialState, ...state })

  useEffect(() => {
    initGTM({
      dataLayer: state.dataLayer,
      dataLayerName: state.dataLayerName,
      environment: state.environment,
      id: state.id
    })
  }, [state])

  return (
    <GTMContext.Provider value={store}>
      <GTMContextDispatch.Provider value={dispatch}>{children}</GTMContextDispatch.Provider>
    </GTMContext.Provider>
  )
}

function dispatchGTMEvent() {
  const context = useContext(GTMContextDispatch)
  if (context === undefined) {
    throw new Error('dispatchGTMEvent must be used within a GTMProvider')
  }

  return context
}

export { GTMProvider, dispatchGTMEvent }

Would be really pleased to hear your thoughts about it!

elgorditosalsero commented 3 years ago

Hey @shtelzerartem,

will look into it as soon as possible 😄

Feel free to open a PR!

elgorditosalsero commented 3 years ago

Hi @shtelzerartem,

sorry for the late update.

I've done a new branch feature/memo wrapping the provider with React.memo.

Could you test it?

Thx

precious-void commented 3 years ago

@elgorditosalsero I'm still experiencing this issue. Have made a pull request with steps to reproduce in description (#18), so you can check it. I don't think wrapping with react memo will help.

elgorditosalsero commented 3 years ago

Hi @shtelzerartem

Sorry for the late answer.

Merged the PR. #19