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

Remove dom-helpers, add local lib/canUseDom #1066

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

tggreene
Copy link
Contributor

This eliminates a dependency on an older version of dom-helpers (3.2.1) which conflicts with that required by react-transition-group (dom-helpers 5.0.1). This addresses an issue using evergreen with shadow-cljs which disallows multiple versions of the same package #858 (I realise it's closed currently, there's a little more detail in my comment here).

I ran tests with ava successfully but did trip over some issues with xo but I believe they're not introduced by my changes.

There are no ui changes with this and the portal appears to work perfectly well with the updated changes.

I wasn't sure if I should update examples/ssr-next or not, I had a bit more difficulty running that with something like an incompatible node version. I can dig further into this if it's helpful and update the yarn.lock there too!

Thanks!

src/portal/src/Portal.js Outdated Show resolved Hide resolved
@tggreene tggreene changed the title Update dom-helpers to 5.0.1 Remove dom-helpers, add local lib/canUseDom Oct 5, 2020
@@ -6,7 +6,7 @@ import GitHubButton from 'react-github-button'
import GitHubIcon from './GitHubIcon'
import SpectrumIcon from './SpectrumIcon'
import LogoWordmark from './LogoWordmark'
// eslint-disable-next-line import/no-unassigned-import
// eslint-disable-next-line import/no-unassigned-import, import/extensions
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

welp, look at me thinking I'm being clever

@tggreene
Copy link
Contributor Author

tggreene commented Oct 6, 2020

I think this is ready for a recheck.

I think I bumped into a cached dependency difference with xo with respect to the above failure! I think when the dependency cache is killed this guy will flare up 👉 xojs/xo@d3abdb6.

This change turned out to be quite noddy in the end, but appreciate your consideration all the same!

Copy link
Contributor

@mshwery mshwery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final change and this looks good to me!! Thank you 🙌

src/lib/canUseDom.js Outdated Show resolved Hide resolved
Remove an unneeded dependency on dom-helpers and simply add a local replacement
for determining that we can use the dom (or essentially that we are in a
browser).
Copy link
Contributor

@mshwery mshwery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! Thanks

@mshwery mshwery merged commit 630fa25 into segmentio:master Oct 6, 2020
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.

None yet

2 participants