-
Notifications
You must be signed in to change notification settings - Fork 62
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
Update tsconfig files #284
Conversation
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.
Hi @Griffin-Sullivan thank you so much. Could you please tweak a little your commit message and also mention that we are organizing some imports?
deb70b6
to
a467999
Compare
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.
Really great job, only a couple of notes here.
@@ -15,18 +15,12 @@ | |||
"forceConsistentCasingInFileNames": true, |
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.
Can you make baseUrl
set to "./src"
?
import { Support } from '@app/Support/Support'; | ||
import { NotFound } from '@app/NotFound/NotFound'; | ||
import { Admin } from '@app/Settings/Admin'; | ||
import { Dashboard } from './Dashboard/Dashboard'; |
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.
with the linter, we should enforce imports to have an absolute path.
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.
Working on the linting while this is in review so I can make sure we set that correctly in my next PR
"node_modules" | ||
"node_modules", | ||
"dist", | ||
"src/__tests__/cypress" |
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.
There's a public library generated by cypress too, have you considered that?
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 know that's what we do in ODH, but when I added the Cypress setup I intentionally removed the separate testing build dir and decided to have our normal build and cypress build use the dist
directory. See the last two cypress commands here https://github.com/kubeflow/model-registry/blob/main/clients/ui/frontend/package.json#L28
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.
We can always change this btw I just thought it was simpler this way. Maybe there's a benefit to having a separate build dir for normal build vs testing build
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 right, that's my bad, was checking the odh dashboard commands, good call here!
@@ -15,18 +15,12 @@ | |||
"forceConsistentCasingInFileNames": true, | |||
"noImplicitReturns": true, | |||
"noImplicitThis": true, | |||
"noImplicitAny": false, | |||
"noImplicitAny": true, |
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.
In the dashboard we have downLevelIteration too, cause it can cause some shenanigans with certain tokens when traspilint to old javascript.
a467999
to
1acb6f4
Compare
…orrectly transpile Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
1acb6f4
to
3a72541
Compare
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.
/lgtm
@lucferbux: changing LGTM is restricted to collaborators In response to this:
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. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, lucferbux The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…kubeflow#284) Bumps [github.com/opencontainers/runc](https://github.com/opencontainers/runc) from 1.1.5 to 1.1.12. - [Release notes](https://github.com/opencontainers/runc/releases) - [Changelog](https://github.com/opencontainers/runc/blob/v1.1.12/CHANGELOG.md) - [Commits](opencontainers/runc@v1.1.5...v1.1.12) --- updated-dependencies: - dependency-name: github.com/opencontainers/runc dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Griffin-Sullivan <gsulliva@redhat.com>
Description
Updating the tsconfig files to reflect what we need to include. Mostly reflecting the same compiler options that ODH uses. Because of some changes to the config file I also had to update some import paths.
How Has This Been Tested?
Merge criteria: