Remove react-json-view to solve downstream security alerts.
#26852
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I couldn't run things locally because of a less error at startup. This fixes that by: - Fixing `@font-size-base`, as it was trying to use a Varnish variable that I didn't seem to have locally. - Replace `0.75rem` with a variable reference. The variable also includes a unit. Either fix in isolation didn't fix the error. I'm not sure why.
Fix the less build.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
This ensures the build occurs prior to deploying. Previously it was failing because `dist_varnish` didn't exist.
Build before deploying.
This removes the `--ssr` flag, which for some reason causes the site build to fail. I'm fairly certain it's because of `site/theme/template/Layout/Header/DemoHeader.tsx`, which is our addition. There's probably something we have to do to make it SSR compatible. I couldn't figure out what's wrong, I tried lots of things (like removing `injectIntl`, etc) but couldn't get to the root issue. That said we're not worried about getting eyeballs on our fork as it's an internal tool, so it seems fine to just disable SSR and call this good.
Several of the sites that we operate that use `@allenai/varnish` are currently flagged with a security alert related to an out of date version of `node-fetch`. The dependency in question traces back to `react-json-view`. This PR removes that dependency entirely. A careful eye might notice that the impacted `node-fetch` version is still around in the `package-lock.json`. That's because `bisheng` depends on an old-version of `react-router` which depends on `create-class` which depends on an old version of `fbjs` which depends on old version of `isomorphic-fetch` which depends on the vulnerable version of `node-fetch`. Yikes, and I probably missed a step or two as that was from memory. That said it's safe to not worry about the fact that it's still around because it's only in `devDependencies` now. That means downstream applications won't be impacted, since they don't transitively include `devDependencies`. So, we net out to a package with one less transitive thing for our clients to suck down. Turns out it wasn't even something that was necessary too -- so we were being a bit careless and should be more diligent about our dependency management moving forward. I also tried to preserve some semblance of the prior UI treatment. While there's some loss of functionality I think the new treatment is totally ok. That said I did add a comment (and will follow up with an issue shortly), as think this is unquestionably a place where our docs can and should be better. Users use the `Theme` API a lot, and there's no type safety for it given it becomes an `any` when it crosses across the `styled-components` boundary. So it's really important for us to make it easy for users to digest.
|
Oops, sorry, this was meant to be opened against our fork. Apologies for the noise! |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 096f3c2:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
View rendered .github/ISSUE_TEMPLATE.md
View rendered .github/PULL_REQUEST_TEMPLATE/pr_cn.md
View rendered README-pt_BR.md
View rendered README-zh_CN.md
View rendered README.md
View rendered components/affix/demo/basic.md
View rendered components/affix/demo/debug.md
View rendered components/affix/demo/on-change.md
View rendered components/affix/demo/target.md
View rendered components/alert/demo/banner.md
View rendered components/alert/demo/basic.md
View rendered components/alert/demo/closable.md
View rendered components/alert/demo/close-text.md
View rendered components/alert/demo/custom-icon.md
View rendered components/alert/demo/description.md
View rendered components/alert/demo/error-boundary.md
View rendered components/alert/demo/icon.md