VictorCazanave / react-svg-map

A set of React.js components to display an interactive SVG map
https://victorcazanave.github.io/react-svg-map/
MIT License
229 stars 48 forks source link

refactor(lib): bump dependencies, adjust dev and build config, refactor tests, refactor components, adjust docs, a.o. #55

Closed mihai-ro closed 2 years ago

mihai-ro commented 3 years ago

Work done:

Close #44 Close #53

VictorCazanave commented 3 years ago

Thanks a lot for your hard work @mihai-ro! I'll probably need long time to review it, because there many changes and I don't work with react for several years, then I'm not really aware of the latest changes šŸ˜…

However, without looking at the code in details, here is a 1st quick review:

  1. I think this PR should be split into smaller ones, because it mixes bug fixes, new features and refactoring. For example: #44 needs to bump to v3 but not #53
  2. The linter configuration shouldn't be modified: prettier is not necessary, I prefer to use tabs over spaces...
  3. In this project, what is the advantage of replacing enzyme with testing-library? It's always interesting to learn new tools, but for projects already published, I favor stability and maintainability.
  4. https://github.com/VictorCazanave/react-svg-map/pull/55/commits/63a70672eaa591adc8b6eb8afc7d54a7a4c3fd57 is a good idea, but CSS variables aren't supported by all the browsers that are supported by react

In general:

However, I know this code is pretty old and I saw interesting ideas in your PR to avoid the code duplication. So, feel free to open another PR only with your code refactoring, and I'll be glad to review it!

mihai-ro commented 3 years ago

I admit, I got carried away with the refactoring, and the linting config is too much šŸ˜…
I will revert the lint config changes and push again.

  1. It is going to be a bit harder to split this into smaller PRs, as the whole React v17 change is simply not a big deal that can be swept away as a refactoring; at the same time we can actually close two issues with a single PR, and then bump the library to v3;
  2. I agree, that is my personal preference :) I will revert to your initial config and remove prettier.
  3. testing-library changes the mindset for testing, as the DX is to focus more on the actual behaviour that you want to test, rather than on how the engine works; e.g. I removed the whole wrapper refresh part, as that is just a flaw that enzyme patched with the wrapper update; additionally, it has full compatibility and no 3rd party non-official plugins are needed to make it actually run. Also, if you look through the syntax, it is actually cleaner and more intuitive; e.g. we don't have to google what keycode is that one that the test executes. I would argue testing-library is the better way to test integrations in react applications.
  4. Hear me out :D - this is just the first step:) in another PR, if this is going to be merged, I intend to provide a boolean prop that would allow users to enable (or disable) default style (e.g. pass hasDefaultStyle to component); using that boolean we can dynamically import that css file with variables; if not, they will just provide their own variables in :root or whatever component they have their variables attached to. Also, we can check whether the browser supports variables. I am aware that variables do not have full coverage over all of the browsers, at the same time I would like to ask whether you intend to continue providing support to browsers such as IE (which is a dead project), or smaller Chinese browsers that actually fell back with their implementations. If not, we can safely use variables.

I would argue bumping dependencies and keeping your application up-to-date is a good practice, and refactoring is not a bad thing, as long as it does not turn the project upside down. On the opposite, failing to update dependencies would potentially expose security flaws. Regarding the bugs - there is no software that is absolutely bug-less, that is why we test and gradually improve the application to make it more sturdy and reliable to the users. With the second bit - I concur:) but that does not mean we just have to lay back and expect things to just work, as this is how we grow, by revisiting our projects and understanding what can be improved.

As I mentioned above, I will try to squeeze some time to get some updates up; it will most likely not be this week, but I will do my best in the near time.

VictorCazanave commented 2 years ago

Sorry for the long silence, I really had no time to work on personal projects šŸ˜©

  1. I understand and thank you again for your contribution, but I can't neither spend so much time reviewing this PR, nor merge and publish code I don't know. Then, I think it's better to close the PR and start working on the v3 step by step. You are also free to fork this project or create your own components, to publish them with the features you want (and probably a better maintenance than mine šŸ˜…)
  2. I know the benefits of testing-library, but I still think it wasn't necessary to replace enzyme here. However, I might consider it, if I have enough time to work on the v3.
  3. I think these components are simple with a limited amount of CSS, then I would prefer to keep them small and not add extra JS only to import a CSS file. I would love to use CSS variables (that I happily use in my everyday work) and kinda share your opinion on the old browsers, but I intend to support the same browsers as React, otherwise IMHO it means that the components aren't fully compatible with the framework. Moreover, I think the CSS is really not the main feature of these components (unlike a drop-down or modal component for example), then it's very easy to write your own CSS.