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

Fix peer dependencies #7900

Merged
merged 9 commits into from
Dec 2, 2024
Merged

Fix peer dependencies #7900

merged 9 commits into from
Dec 2, 2024

Conversation

tempoz
Copy link
Contributor

@tempoz tempoz commented Nov 14, 2024

This addresses all peer dependency warnings we get from yarn except for the dagre-d3-react ones, which would require a new release of that library to fix.

In doing so, it:

  • Upgrades our typescript version

  • Switches to using useResizeDetector from the deprecated withResizeDetector

  • Switches to using Range instead of the removed RangeWithKey and RangeKeyDict instead of the removed OnChangeProps

  • Removes code for handling the days field of a Range, as that field no longer exists

  • Removes the undefined second positional parameter from handleDefaultConfigClicked, as that function no longer accepts a second positional parameter

  • Removes fallback values when using unary + to convert a Long to a number, as that now always returns a number

  • Changes our target in tsconfig.json from es2016 to es2018 to support the use of named capturing groups in trace_events.ts

  • Removes the resolutions block from our package.json, as it is no longer necessary

@tempoz tempoz requested a review from bduffany November 14, 2024 21:52
Comment on lines 521 to 525
const { width, ref } = useResizeDetector({
handleHeight: false,
refreshMode: "throttle",
refreshRate: 500,
});
Copy link
Member

Choose a reason for hiding this comment

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

does this work? I thought React hooks (functions prefixed with use) only worked with functional components, and weren't supported for class components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then you're probably right; I couldn't test this locally and so was going to test it in dev when I got a free moment. How do you recommend we resolve this? The library just removed the with version, and I wasn't aware of the differing use in class and functional components until just now.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd ignore the deprecation warning

Copy link
Member

Choose a reason for hiding this comment

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

(and revert to the old version - unless the issue is that this is setting off some security scanner?)

Copy link
Member

Choose a reason for hiding this comment

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

i also wouldn't expect this to work. if you can't / don't want to revert versions and figure out the class-componenty way of doing this, you should be able to just set an olap db handle to play around with the ui (since you don't need any actual data to verify that resizing is working)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the HeatmapComponent to a functional component instead of a class component, and it appears to work based on my limited testing I did on my local machine with no real data.

@bduffany bduffany requested a review from jdhollen November 15, 2024 20:10
@@ -265,11 +265,11 @@ export default class FlakesComponent extends React.Component<Props, State> {
if (!stats) {
return "0%";
}
const totalFlakes = (+stats.flakyRuns ?? 0) + (+stats.likelyFlakyRuns ?? 0);
const totalFlakes = +stats.flakyRuns + +stats.likelyFlakyRuns;
Copy link
Member

Choose a reason for hiding this comment

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

did something change in our proto libraries to make this not a Long or something? if so, i would imagine there are a lot more places than just this one where this could be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it made it so that Longs can't fail to be converted to numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also I think this is likely a result of upgrading typescript, not proto

Comment on lines 521 to 525
const { width, ref } = useResizeDetector({
handleHeight: false,
refreshMode: "throttle",
refreshRate: 500,
});
Copy link
Member

Choose a reason for hiding this comment

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

i also wouldn't expect this to work. if you can't / don't want to revert versions and figure out the class-componenty way of doing this, you should be able to just set an olap db handle to play around with the ui (since you don't need any actual data to verify that resizing is working)

enterprise/app/filter/date_picker_button.tsx Outdated Show resolved Hide resolved
@tempoz tempoz force-pushed the fix-peer-deps branch 2 times, most recently from eb16e16 to 5686ecd Compare November 26, 2024 19:13
@tempoz tempoz requested a review from jdhollen November 27, 2024 19:53
@tempoz tempoz merged commit 70ef6f7 into master Dec 2, 2024
15 checks passed
@tempoz tempoz deleted the fix-peer-deps branch December 2, 2024 20:02
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.

3 participants