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

Update the eslint config and pnpm version in order to fix the build #327

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

JackRobards
Copy link
Contributor

@JackRobards JackRobards commented Sep 13, 2023

The build is failing across all of the PRs on this repo, preventing any new changes from being merged. e.g. https://github.com/CrowdStrike/tailwind-toucan-base/actions/runs/6162755087/job/16726205458?pr=326

I believe this error ultimately has to do with the CI installing TypeScripting 5.2, which is incompatible with the older eslint setup in this repo. See: typescript-eslint/typescript-eslint#7155 (you can see the same error message in a comment on that thread). The property this error refers to was deprecated in 5.0, and is no longer usable in 5.2: https://github.com/microsoft/TypeScript/blob/main/src/deprecatedCompat/5.0/identifierProperties.ts#L28

The main goal here was to get the build working by updating the versions, since it seems like it's been awhile since the build config for this repo was updated. I think we could also lock the typescript version to "typescript": "5.1.6", explicitly to fix the build, but the eslint changes would need to be made in the future anyway once we want to update TypeScript.

Notable changes include:

  • Update TypeScript to 5.2 in the main package, and tests directory (it still listed 4.7, and did not get updated by renovate).
  • Updated the eslintconfig from nullvoxpopuli. The new node config: https://github.com/NullVoxPopuli/eslint-configs/blob/main/configs/node.js#L127
  • Use the same pnpm version across CI, the volta pinned version, and packageManager. Renovate only updated the pnpm dependency, and not these other versions so things were a bit out of sync and confusing.

Let me know what you think!

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2023

⚠️ No Changeset found

Latest commit: c074097

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JackRobards JackRobards changed the title WIP: Updating things and fixing the build Update the eslint config and pnpm version in order to fix the build Sep 13, 2023
@@ -9,7 +9,7 @@
"masterIssue": true,
// bump for apps
// update-lockfile for addons/libraries
"rangeStrategy": "bump",
"rangeStrategy": "update-lockfile",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping this might help with keeping this more consistent across different open source CrowdStrike repos. Both ember-toucan-styles and ember-toucan-core are using the "update-lockfile" strategy. I'm not too familiar with configuring renovate though, so let me know if this is a bad idea!

Docs:
https://docs.renovatebot.com/configuration-options/#rangestrategy

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me to keep these all consistent 👍

@@ -81,6 +84,6 @@
"packageManager": "[email protected]",
"volta": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For anyone who might be curious how volta respects (or doesn't for now) this packageManager field, I found these issues with some context:

Copy link
Contributor

@ynotdraw ynotdraw left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this.

@@ -9,7 +9,7 @@
"masterIssue": true,
// bump for apps
// update-lockfile for addons/libraries
"rangeStrategy": "bump",
"rangeStrategy": "update-lockfile",
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me to keep these all consistent 👍

- The CI pnpm version was out of sync with the package.json version.
- The tests app was using older versions of typescript and vitest
- The .eslintignore was missing a build directories, resulting in slow linting times
…e build

- Bump TypeScript version, since CI wasn't using a frozen lockfile for TypeScript version.
- Add "rangeStrategy": "update-lockfile", for renovate settings
- Various small fixes to adjust to new eslint settings
To fix an error in conflicting versions of @types/node
@JackRobards JackRobards force-pushed the jrobards/bump-pnpm-version branch from 3685d28 to c074097 Compare September 13, 2023 16:15
@ynotdraw ynotdraw merged commit a4e2282 into CrowdStrike:main Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants