-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Deps update 20210628 #1680
Deps update 20210628 #1680
Conversation
e1f6372
to
3ffe8e2
Compare
In all of these dash core dep upgrades (here, plotly/dash-html-components#194, plotly/dash-core-components#939, plotly/dash-table#918) I used node 16.4.0 and npm 7.18.1. This introduced a package-lock format change - which is backward-compatible but still causes warnings if you later install these packages using npm 6 & node 14. But the biggest effect of this is stricter peer dep checks, and some of the packages we depend on still list only react 16 and below as peer deps. This actually caused me to downgrade react from 17 to 16 in both dcc and table. Our tests passed with 17, so it appears this check is in reality overzealous, but as we're still using 16 in dash I thought it better to play it safe, and when we want to bump to 17 everywhere we can revisit this, with any luck the relevant packages will have been updated by then. |
@@ -125,7 +125,7 @@ const observer: IStoreObserverDefinition<IStoreState> = { | |||
exg => Boolean(exg), | |||
pluck('executionGroup', groupWithoutInitial) | |||
).slice(-1)[0] | |||
}, | |||
} as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure, we want to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right that we should try to minimize escapes like this, but the upgraded TypeScript barfed before this change and I couldn't find a better solution easily. IIRC the issue is .slice(-1)[0]
could fail to yield an element, except that we know from how we constructed groupWithoutInitial
that there will be at least one. @eff-kay perhaps at some point (after this release) you can take on the @types/ramda
bump and also clean this up? I'm definitely not a power TS user, and it seems like Ramda pushes TS pretty hard.
@@ -413,6 +413,7 @@ def get_webdriver(self): | |||
return getattr(self, "_get_{}".format(self._browser))() | |||
except WebDriverException: | |||
logger.exception("<<<Webdriver not initialized correctly>>>") | |||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can move the return statement to outside of the exception, thereby returning a None in all scenarios where the try fails.
Not sure if that is intended though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's better, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though actually, if the try fails for any reason other than WebDriverException
the error will be thrown so we'll never reach that return. I'll leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes thats what I had in mind. If there is a possibility that the try block will fail because of other causes besides WebDriverException
, then we should probably add a generic except
block at the end, with the relevant logging and return.
LGTM, most of the changes look like code formatting and dependency upgrades. |
Note: the
black==21.6b0;python_version>="3.0"
update will need to percolate out to the rest of the core packages - working on that now.One thing I punted on was ramda typescript binding: a whole bunch of errors popped up when I tried to update this so I pinned it to
"@types/ramda": "0.27.6"
rather than letting it float to 0.27.41