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

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

Closed
wants to merge 24 commits into from

Conversation

mihai-ro
Copy link

@mihai-ro mihai-ro commented Sep 12, 2021

Work done:

  • refactor(deps, tests): replace enzyme with testing-library and adjust config. update tests. bump all dependencies to latest versions
  • ci(babelrc): adjust babelrc to reference new babel plugins
  • ci(webpack): update webpack config to v5. adjust rules and extend production config
  • refactor(map components): use functional components. use refs and forward refs as needed for flexibility purposes
  • style(svg-map): extract colors into a separate css file. set default component classname to svg-map and concat custom
  • docs(examples): remove unnecessary bindings in a class component
  • docs(readme): use PureComponents in examples. fix typo
  • refactor(misc): setup babel to no longer require importing React in every file
  • ci(eslintrc): add hook linter. turn off warnings about importing React as no longer needed
  • feat(checkbox-map): rebuild checkbox map and provide both controlled and uncontrolled component options.
  • feat(radio-svg-map): rebuild radio-map and provide both controlled and uncontrolled component options
  • refactor(examples): refactor examples to show controlled variant of the maps
  • refactor(tests): refactor tests to thoroughly check controlled variants of the components
  • chore(proptypes): extract proptypes and reuse them in all of the components
  • docs(readme): remove import React from examples as it is no longer needed in React >v17
  • docs(readme): update readme and describe defaultValue, value and onChange

Close #44
Close #53

@mihai-ro mihai-ro changed the title refactor(lib): resuscitate the library refactor(lib): bump dependencies, adjust config, refactor components, adjust docs Sep 12, 2021
@mihai-ro mihai-ro changed the title refactor(lib): bump dependencies, adjust config, refactor components, adjust docs refactor(lib): bump dependencies, adjust dev and build config, refactor tests, refactor components, adjust docs, a.o. Sep 12, 2021
Mihai Ro and others added 15 commits September 13, 2021 15:29
@VictorCazanave
Copy link
Owner

VictorCazanave commented Sep 19, 2021

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: Can we get selectedLocationIds to update on re-render #44 needs to bump to v3 but not RadioSVGMap broken in React 17 #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. 63a7067 is a good idea, but CSS variables aren't supported by all the browsers that are supported by react

In general:

  • if there is no security or performance issue, I think it's not necessary to update the dependencies, because it may create new bugs
  • if there is no bug, missing feature or performance issue, I think it's not necessary to modify the code, otherwise the refactoring will never stop 😄

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
Copy link
Author

mihai-ro commented Sep 20, 2021

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
Copy link
Owner

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.

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.

RadioSVGMap broken in React 17 Can we get selectedLocationIds to update on re-render
2 participants