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

Patch react-awesome-styled-grid #459

Merged
merged 1 commit into from
Apr 13, 2022
Merged

Patch react-awesome-styled-grid #459

merged 1 commit into from
Apr 13, 2022

Conversation

nowyDEV
Copy link
Member

@nowyDEV nowyDEV commented Apr 12, 2022

What

  • Patched usage of global in react-awesome-styled-grid, it was causing problems in browser & test environment

Compatibility

  • Does this change maintain backward compatibility?

Screenshots

Before

After

@nowyDEV nowyDEV added the bug Something isn't working label Apr 12, 2022
@nowyDEV nowyDEV self-assigned this Apr 12, 2022
@nowyDEV nowyDEV requested a review from a team as a code owner April 12, 2022 14:46
@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aws-amplify-eu-west-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-459.du3gff836giqw.amplifyapp.com

@roertbb
Copy link
Contributor

roertbb commented Apr 13, 2022

Could you elaborate on problems in browser & test environment?

From what I see, root cause of the problem comes from https://github.com/donavon/use-event-listener.

I'm aware that sometimes patching deps may be necessary, but I'd rather use it as a last resort and leave extensive comment why it's done to have full context on how to get rid of it in the future

@nowyDEV
Copy link
Member Author

nowyDEV commented Apr 13, 2022

Could you elaborate on problems in browser & test environment?

From what I see, root cause of the problem comes from https://github.com/donavon/use-event-listener.

I'm aware that sometimes patching deps may be necessary, but I'd rather use it as a last resort and leave extensive comment why it's done to have full context on how to get rid of it in the future

It uses global that becomes undefined both in the browser & in test environment

santosfrancisco/react-awesome-styled-grid#138
vitejs/vite#728 (comment)
Screenshot_2022-04-13_12-56-57

This causes test failures with vitest (cannot add event listener to undefined)

Screenshot_2022-04-13_14-44-07

react-awesome-styled-grid seems to no longer be actively maintained (last release september 2021), the risk of update that will break the patch is unlikely (it's pretty easy to modify the patch anyway)

@roertbb
Copy link
Contributor

roertbb commented Apr 13, 2022

Could you elaborate on problems in browser & test environment?
From what I see, root cause of the problem comes from https://github.com/donavon/use-event-listener.
I'm aware that sometimes patching deps may be necessary, but I'd rather use it as a last resort and leave extensive comment why it's done to have full context on how to get rid of it in the future

It uses global that becomes undefined both in the browser & in test environment

santosfrancisco/react-awesome-styled-grid#138 vitejs/vite#728 (comment) Screenshot_2022-04-13_12-56-57

This causes test failures with vitest (cannot add event listener to undefined)

Screenshot_2022-04-13_14-44-07

react-awesome-styled-grid seems to no longer be actively maintained (last release september 2021), the risk of update that will break the patch is unlikely (it's pretty easy to modify the patch anyway)

Thanks for detailed context 😉
From what I see, vitest seems to be able to configure global API for tests - see: https://vitest.dev/config/#globals.
Same for vite it seems that are other workarounds that are not patching libs - aws-amplify/amplify-js#9639 (comment).
I don't wanna block the PR, since 2 other guys approved it, but I feel we may have similar problems with other libs and maybe the links above may suggest alternative solution to tackle the problem 😉

@nowyDEV nowyDEV merged commit a1fa9a3 into master Apr 13, 2022
@nowyDEV nowyDEV deleted the patch-global-var branch April 13, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants