(chore) Dependency upgrades and improvements to linting and types #190
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
denniskigen
force-pushed
the
chore/lint-and-type-improvements
branch
from
96a7dcb
to
f790633
Compare
|
Size Change: +42.9 kB (+1%) Total Size: 2.98 MB
ℹ️ View Unchanged
|
denniskigen
force-pushed
the
chore/lint-and-type-improvements
branch
from
f790633
to
c1b185a
Compare
denniskigen
force-pushed
the
chore/lint-and-type-improvements
branch
from
c1b185a
to
dbbb86d
Compare
ibacher
reviewed
Sep 18, 2023
| "extends": [ | ||
| "eslint:recommended", | ||
| "plugin:prettier/recommended", | ||
| "plugin:@typescript-eslint/recommended", | ||
| "ts-react-important-stuff" |
.prettierrc
Outdated
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "bracketSpacing": true, | |||
| "printWidth": 80, | |||
There was a problem hiding this comment.
Yeah. I'll switch back to that.
webpack.config.js
Outdated
| "@openmrs/openmrs-form-engine-lib": | ||
| "@openmrs/openmrs-form-engine-lib/src/index", | ||
| '@openmrs/esm-framework': '@openmrs/esm-framework/src/internal', | ||
| '@openmrs/openmrs-form-engine-lib': |
There was a problem hiding this comment.
I know this wasn't added in this PR, but it's absolutely absurd that we need an alias for this. We should be able to setup the package.json for the form engine lib to remove this necessity (things are a little different with esm-framework; we have a series of "internal" methods, which are basically exposed so they can be called from the app-shell, but nowhere else, but at runtime, we only want one framework bundle, so this rule has us bundle the full esm-framework, including internals, but only allow user-code to see the public interface).
.eslintignore
Outdated
| @@ -1,2 +1,3 @@ | |||
| e2e/** | |||
There was a problem hiding this comment.
Shouldn't the tests themselves be linted?
There was a problem hiding this comment.
Yes, they should. I forgot to restore this.
ibacher
previously approved these changes
Sep 19, 2023
|
Dunno why the e2e tests are failing @ibacher. I've posted a thread on the qa-support channel on Slack. |
ibacher
reviewed
Sep 19, 2023
e2e/specs/forms-dashboard.spec.ts
Outdated
| .getByRole("combobox", { | ||
| name: "Filter by publish status: All Open menu", | ||
| .getByRole('combobox', { | ||
| name: 'Filter by publish status: All Open menu', |
There was a problem hiding this comment.
I don't think this test could've ever succeeded. The DOM element with the role combobox is the button which only has the text "All".
There was a problem hiding this comment.
The Playwright codegen tool suggests using page.getByText('Filter by publish status:')
playwright-codegen.mp4
| await page.getByRole('row', { name: form.name }).getByLabel('Edit Schema').click(); | ||
| }); | ||
|
|
||
| await test.step("Then I click on the unpublish form button and confirms the unpublication", async () => { | ||
| await test.step('Then I click on the unpublish form button and confirms the unpublication', async () => { | ||
| await formBuilderPage.unpublishFormButton().click(); | ||
| await formBuilderPage.unpublishFormConfirmationButton().click(); |
There was a problem hiding this comment.
Watching the video, this doesn't seem to work. There's a failure "Cannot read properties of undefined (reading 'length')". This seems to trigger at the first unpublishFormButton().click().
|
All fixed, @ibacher. |
vasharma05
previously approved these changes
Sep 20, 2023
.husky/pre-commit
Outdated
| npx lint-staged |
There was a problem hiding this comment.
We have the pretty-quick as well in the pre-commit
There was a problem hiding this comment.
Fixed in the latest commit
donaldkibet
previously approved these changes
Sep 20, 2023
denniskigen
dismissed stale reviews from donaldkibet and vasharma05
via
9498255
vasharma05
approved these changes
Sep 20, 2023
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.
Requirements
Summary
I’ve made various upgrades to dependencies, lint tooling and types in this PR. Specifically, these changes include:
pretty-quickwithlint-stagedfor managing pre-commit hooks.pretty-quickisn’t compatible with the latest version of Prettier.Other
Changes to linting and formatting were inspired by https://www.joshuakgoldberg.com/blog/configuring-eslint-prettier-and-typescript-together/.