-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[ui] Clean up yarn peer dependency spew #18968
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit 4a2667b. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 4a2667b. |
499f953
to
4a2667b
Compare
Also upgraded TypeScript while fixing the build error. |
@@ -16,7 +16,7 @@ const config = { | |||
getAbsolutePath('@storybook/addon-mdx-gfm'), | |||
], | |||
framework: { | |||
name: getAbsolutePath('@storybook/nextjs'), | |||
name: '@storybook/react-webpack5', |
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.
oh weird that we needed to change this, did you make sure nothing broke with storybook?
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.
Yeah, it looks fine. I think this might have been automated during the storybook migration, but since ui-core
doesn't know anything about Next, it doesn't have the next
dependency, so it's not quite the right framework option here.
@@ -12,8 +12,8 @@ export const TimeElapsed = (props: Props) => { | |||
const {startUnix, endUnix} = props; | |||
|
|||
const [endTime, setEndTime] = React.useState(() => (endUnix ? endUnix * 1000 : null)); | |||
const interval = React.useRef<NodeJS.Timer | null>(null); | |||
const timeout = React.useRef<NodeJS.Timer | null>(null); | |||
const interval = React.useRef<NodeJS.Timeout | null>(null); |
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.
I realize this wasn't added in this PR but why is this NodeJS.Timeout ? We're not running this code in NodeJS
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.
Yeah, agree, this is a little weird. I think it should be number
, but maybe there's something we need to do in our tsconfig to make that behave properly. I'll see if I can fix it in a followup.
## Summary & Motivation Clean up peer dependency logspew during Yarn installation. I'll try to take care of the Blueprint one in a followup. Output: ``` (dagenv310) dish dagster-ui $ yarn ➤ YN0000: · Yarn 4.0.2 ➤ YN0000: ┌ Resolution step ➤ YN0000: └ Completed in 0s 321ms ➤ YN0000: ┌ Post-resolution validation ➤ YN0060: │ react is listed by your project with version 18.2.0, which doesn't satisfy what @blueprintjs/core (p1eae0) and other dependencies request (but they have non-overlapping ranges!). ➤ YN0060: │ react-dom is listed by your project with version 18.2.0, which doesn't satisfy what @dagster-io/ui-core (ped706) and other dependencies request (but they have non-overlapping ranges!). ➤ YN0086: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code. ➤ YN0000: └ Completed ➤ YN0000: ┌ Fetch step ➤ YN0000: └ Completed in 0s 446ms ➤ YN0000: ┌ Link step ➤ YN0000: └ Completed in 0s 752ms ➤ YN0000: · Done with warnings in 1s 661ms ``` ## How I Tested These Changes Yarn, buildkite.
Summary & Motivation
Clean up peer dependency logspew during Yarn installation. I'll try to take care of the Blueprint one in a followup.
Output:
How I Tested These Changes
Yarn, buildkite.