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

dependency maintenance #354

Merged
merged 10 commits into from
Jul 11, 2023
Merged

dependency maintenance #354

merged 10 commits into from
Jul 11, 2023

Conversation

nickgros
Copy link
Collaborator

@nickgros nickgros commented Jul 6, 2023

Ran pnpm update --recursive and followed up with a few manual version changes to achieve the following

  • Fix all issues that appear in pnpm audit that have an upgrade path
  • Fix all but one peer dependency issues
  • Remove project-specific resolutions fields (these had no effect on dependency resolution)
  • Small changes to account for package updates

Additionally, update our SBOM upload workflow to continue getting vulnerability alerts (still needed while we wait on dependabot/dependabot-core#7434).

@@ -28,6 +27,6 @@ jobs:
name: sbom
path: _manifest/spdx_2.2
- name: SBOM upload
uses: jhutchings1/[email protected]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

original repo was archived and points to this new action

Comment on lines -106 to -113
"resolutions": {
"js-yaml": "3.13.1",
"react": "18.2.0",
"react-hot-toast": "2.2.0",
"@types/react": "18.0.17",
"@types/react-dom": "18.0.6",
"minimatch": "^3.1.2"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the output of pnpm install, these resolution entries are ignored. Only the base package.json resolutions are used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See docs for overrides: https://pnpm.io/package_json#pnpmoverrides - this is the same feature, just a different property name.

- Bump dependencies shown in `pnpm audit` that have valid patches
- Update SBOM workflow for security updates
- Remove project-specific `resolutions` fields (these had no effect on dependency resolution)
@@ -46,6 +46,7 @@ type StandaloneQueryWrapperOwnProps = {
| 'columnAliases'
| 'noContentPlaceholderType'
| 'showLastUpdatedOn'
| 'visibleColumnCount'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was missing in the prop signature, but the prop was still passed down, so this is a type-only change.

@@ -158,6 +158,7 @@ export function useCreateLockAccessRequirement(
if (options?.onSuccess) {
return options.onSuccess(data, variables, ctx)
}
return
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These functions must have a return statement. If I had to guess, it probably has something to do with this TypeScript change and some (outdated) react-query types.

@@ -6,7 +6,7 @@ import defaultMuiThemeOptions from './DefaultTheme'
import type { PartialDeep } from 'type-fest'

export function mergeTheme(
themeOverrides: PartialDeep<ThemeOptions>,
themeOverrides: ThemeOptions | PartialDeep<ThemeOptions>,
Copy link
Collaborator Author

@nickgros nickgros Jul 10, 2023

Choose a reason for hiding this comment

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

MUI/Emotion update required this change. Some change in their types makes ThemeOptions not satisfy PartialDeep<ThemeOptions>. May be a bug in type-fest, which provides the PartialDeep utility type.


export default defineConfig({
export const config: UserConfig = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some changes in types required separating the config object from the config definition.

"react-easy-crop": "^4.2.0",
"react-query": "3.39.1",
"react-router-dom": "5.3.3",
"react-spinners": "^0.11.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped react-spinners as it is unused in this project.

@@ -1,4 +1,4 @@
import { renderHook } from '@testing-library/react-hooks'
import { renderHook } from '@testing-library/react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Functionality in @testing-library/react-hooks has been ported to @testing-library/react, so we dropped @testing-library/react-hooks

@@ -36,14 +36,13 @@ describe('usePreFetchResource tests', () => {
),
)

const { result, waitForNextUpdate } = renderHook(() =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

waitForNextUpdate is not in @testing-library/react, but that's ok, we can just use waitFor

Copy link
Collaborator Author

@nickgros nickgros Jul 10, 2023

Choose a reason for hiding this comment

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

Dropped an <?xml > tag that was not parseable by upgraded esbuild

@nickgros nickgros added the chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests label Jul 11, 2023
Comment on lines -28 to -29
const SynapseClient = require('../../../../src/synapse-client/SynapseClient')
SynapseClient.getEntityBundleV2 = jest.fn().mockResolvedValue(expected)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced dynamic require import with jest.spyOn


const queryClient = new QueryClient()

const wrapper = (props: { children: React.ReactChildren }) => (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replace with existing utility

})

await waitFor(() => result.current.isSuccess)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The returned waitFor must have allowed returning a boolean. The imported waitFor check doesn't work unless an error is thrown, such as if an expect fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No meaningful config changes, just updated to work with new types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Config itself unchanged, just reformatted and structure changed to work with type changes

Copy link
Collaborator Author

@nickgros nickgros Jul 11, 2023

Choose a reason for hiding this comment

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

This workflow pushes a dependency list from our lockfile to a Microsoft tool that generates and reports security vulnerabilities. It's our workaround to get vulnerability alerts until GitHub supports Dependabot security updates for pnpm.

"vite": "^4.3.4",
"tslib": "^2.6.0",
"type-fest": "^3.13.0",
"typescript": "5.1.6",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Upgraded TS everywhere

@@ -56,27 +55,27 @@
"markdown-it-strikethrough-alt": "^1.0.0",
"markdown-it-sub-alt": "^1.0.0",
"markdown-it-sup-alt": "^1.0.2",
"markdown-it-synapse": "^1.1.7",
"markdown-it-synapse": "workspace:^",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using workspace markdown-it-synapse for devDependencies, but I think all of the apps are pulling an old version from CDN. Since the bundling strategy changed, that should be tackled separately

"react-cookie": "4.0.0",
"react-dom": "^18.2.0",
"react-query": "3.39.1",
"react-router-dom": "5.3.3",
"react-spinners": "^0.11.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

react-spinners also unused here

"memfs": "^3.4.7",
"msw": "^0.49.3",
"memfs": "^3.5.3",
"msw": "^1.2.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bump to match other packages

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to .ts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed from .tsx, bad diff

@nickgros nickgros merged commit 1000526 into Sage-Bionetworks:main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromatic Deploys the latest commit on this PR to Chromatic and runs snapshot tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants