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

chore(frontend): Update package.json to be compatible with npm install #8092

Closed
wants to merge 4 commits into from
Closed

chore(frontend): Update package.json to be compatible with npm install #8092

wants to merge 4 commits into from

Conversation

zpChris
Copy link
Contributor

@zpChris zpChris commented Aug 2, 2022

Description of your changes:

  • Update package.json by removing:
    • All storybook dependencies due to conflicting babel-loader dependencies
    • license-checker to circumvent the 403 warning described under "Notes"
    • react-scripts since the craco package already contains a react-scripts version
  • Remove stories due to conflicting babel-loader dependencies, done through this process
  • Update the URLParser.test.ts file to fix failing tests
  • Update snapshot tests

Notes:

  • The normal development method is npm ci; however, we are making these fixes as npm ci runs into a 403 error on my development environment, likely because a limited amount of access has been granted for the following service:
GET https://artifactory.twitter.biz/api/npm/js-virtual/treeify/-/treeify-1.1.0.tgz
  • I assume npm ci is still the desired setup method, as there could be additional unencountered errors with npm install since it ensure the installed dependencies have the same versions as all others.

Checklist:

Comment on lines 21 to 24
const location = {
pathname: '',
search: '',
} as any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

createLocation is no longer a function from history, so I just used a based object instead.

@@ -66,9 +69,9 @@ describe('URLParser', () => {
it('does not create new state when setting query param in-place', () => {
location.search = '?searchkey=searchvalue';

const historyLength = history.length;
const historyLength = window.history.length;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The length property of history is no longer available, so I replaced it here with window.history. Since window.history does not have the search property, I did not replace the full history object, just the instances that used length.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from zijianjoy by writing /assign @zijianjoy in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow
Copy link

@zpChris: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kubeflow-pipeline-frontend-test 8768fce link true /test kubeflow-pipeline-frontend-test
kubeflow-pipeline-e2e-test 8768fce link true /test kubeflow-pipeline-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@zpChris
Copy link
Contributor Author

zpChris commented Aug 3, 2022

We are closing this PR because the kubeflow-pipeline-frontend-test is failing with the following command / error:

eslint --ext js,ts,tsx src
sh: 1: eslint: not found

We tried adding eslint and eslint-config-react-app (as suggested here), but since that did not solve the problem, we are putting this update on hold (and I am re-configuring my workstation via a new GCE VM).

These errors connect to the previous PR #7144; we still hope to remove license-checker to resolve the original issue of the following command giving a 403:

GET https://artifactory.twitter.biz/api/npm/js-virtual/treeify/-/treeify-1.1.0.tgz

However, given time constraints on my internship I am closing this PR for now to work on other tasks to complete the KFPv2 Run Comparison page.

@zpChris zpChris closed this Aug 3, 2022
@zpChris zpChris deleted the update-package-dependencies branch August 3, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants