fingerprintjs / fingerprintjs-pro-react

Fingerprint Pro Wrapper for React Single Page Applications (SPA)
MIT License
52 stars 8 forks source link

Synchronous env detection via `<WithEnvironment />` #61

Closed molefrog closed 2 years ago

molefrog commented 2 years ago

Resolves #59

This PR implements <WithEnvironment /> component which can be used to detect the current env. It has an improved check for Preact that relies on the number of arguments passed down to render() and it doesn't require a re-render. I haven't wired it up with the rest of the code yet (I plan to do that in the next PR), as there are some caveats we need to discuss first:

  1. It uses a class keyword which could potentially increase the bundle size since we target es5 in the config. I don't think it would create any issues though, but proper bundle analysis is probably required
  2. There is no proper test to check if the argument condition does indeed return true when rendered via Preact. I guess that's the responsibility of a more higher-level integration test, ideally covered by <FpjsProvider /> tests, but I didn't want to overload the PR and wanted to make it more atomic.
TheUnderScorer commented 2 years ago

Thanks for the PR @molefrog! I don't have any comments so far, everything looks good.

Let's keep this PR as a "feature branch" so that you can target your next PRs to this one, and once everything is ready we can merge it into a single release.

molefrog commented 2 years ago

Let's keep this PR as a "feature branch" so that you can target your next PRs to this one, and once everything is ready we can merge it into a single release.

Good, I was actually going to propose the same thing. I'll try to get the next one ready early next week.

molefrog commented 2 years ago

Hi @TheUnderScorer, I've just updated the branch with the changes we discussed:

TheUnderScorer commented 2 years ago

Thanks for the update @molefrog!

I like the preact test suite implementation, nice work!

molefrog commented 2 years ago

@molefrog I've fixed all of the comments above, including the removal of SyntheticEventDetector which is not being used anymore in the code.

TheUnderScorer commented 2 years ago

Thanks, @molefrog! From your perspective, do you think that there is anything left to do in regard of this PR?

molefrog commented 2 years ago

Thanks, @molefrog! From your perspective, do you think that there is anything left to do in regard of this PR?

Well, I have tested all examples in the repo with a real API key, works as expected.

There is one additional commit I haven't added here, which is supposed to add an extra check before we call cloneElement in WithEnvironment. Calling cloneElement on plain children (text, booleans etc.) breaks the render, however our types don't allow such children.

https://github.com/molefrog/fingerprintjs-pro-react/commit/215d674ad462e6696a8605501d3cd4c638fc50d8

I think we can safely ignore this, but it is up to you.

molefrog commented 2 years ago

Hi! Just curious if there is anything else that I should consider working on in this PR 😃 I have some ideas for further improvements, but I just don't want overload this PR.

TheUnderScorer commented 2 years ago

Hel @molefrog!

For now, I think that we can consider it finished 😃. Sorry for the overall delay, I was on holiday for the last couple of days.

ilfa commented 2 years ago

:tada: This PR is included in version 1.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: