search

rangle / redux-beacon

Analytics integration for Redux and ngrx/store
https://rangle.gitbook.io/redux-beacon/
MIT License
668 stars 71 forks source link

Warn instead of Throw on missing GA snippet. #343

Closed vonkanehoffen closed 5 years ago

vonkanehoffen commented 5 years ago

Throwing an error here breaks subsequent redux store actions in some cases. It should just be a warning as any underlying React app can function without Google Analytics. .....and GA snippets can easily be removed by careless sysadmins.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

What was done

Changed throw to console.warn so this module doesn't break a whole app when GA isn't loaded :-)

Associated Issues

N/A

:heart: Thanks

Thanks for taking the time to help out with the project, it's much appreciated :slightly_smiling_face:

vonkanehoffen commented 5 years ago

.....why is console.warn not allowed in the CI tests?

vonkanehoffen commented 5 years ago

Thanks @ttmarek ! My first contribution to a public NPM module ever. I feel...... so grown up lol ;-D Sorry I didn't know about the Typescript + CI tooling to sort the tests.

ttmarek commented 5 years ago

Ha! Welcome to the world of OSS!

Sorry I didn't know about the Typescript + CI tooling to sort the tests.

No problem @vonkanehoffen. Thanks for making the pull request! We had made a similar change to yours in some of our other targets so I updated your branch to match what was done elsewhere for consistency.

The fix should be available in the latest version of @redux-beacon/google-analytics feel free to give it a whirl 🙂

Thanks again.