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 lint configs #4

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Update lint configs #4

merged 5 commits into from
Feb 29, 2024

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Feb 29, 2024

Blocked by #3.

This updates all files to match the MetaMask code style. I've updated the ESLint, Prettier, and TypeScript configs, and fixed all lint errors.

@Mrtenz Mrtenz changed the title Mrtenz/metamask code style Update lint configs Feb 29, 2024
mcmire
mcmire previously approved these changes Feb 29, 2024
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

Base automatically changed from mrtenz/yarn to main February 29, 2024 22:23
@Mrtenz Mrtenz dismissed mcmire’s stale review February 29, 2024 22:23

The base branch was changed.

@Mrtenz Mrtenz force-pushed the mrtenz/metamask-code-style branch from c5894e6 to 519adea Compare February 29, 2024 22:24
@Mrtenz Mrtenz marked this pull request as ready for review February 29, 2024 22:24
Copy link

socket-security bot commented Feb 29, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

mcmire
mcmire previously requested changes Feb 29, 2024
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Oops, sorry, I didn't notice the CI failure.

"repository": "git://github.com/MetaMask/superstruct.git",
"type": "module",
Copy link

@mcmire mcmire Feb 29, 2024

Choose a reason for hiding this comment

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

I see that we're no longer using ESM. Is that intentional? Should we stick with ESM in this PR and then remove it in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm planning to swap out Rollup for tsup using the same configs we use in the module template, which doesn't use type: module.

"exactOptionalPropertyTypes": true,
"forceConsistentCasingInFileNames": true,
"lib": ["ES2020"],
"module": "CommonJS",
Copy link

Choose a reason for hiding this comment

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

Getting an error from Rollup. It looks like module and moduleResolution need to remain as esnext, not CommonJS, at least in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to fix Rollup, but it seems like it's more effort than it's worth at this point since I'm planning to remove it in the next PR. Changing module and moduleResolution alone doesn't seem to fix it at least.

@mcmire mcmire dismissed their stale review February 29, 2024 22:35

We'll be removing Rollup later anyway.

@Mrtenz Mrtenz merged commit 8fb18c6 into main Feb 29, 2024
4 of 8 checks passed
@Mrtenz Mrtenz deleted the mrtenz/metamask-code-style branch February 29, 2024 22:40
@Mrtenz Mrtenz mentioned this pull request Mar 1, 2024
Mrtenz added a commit that referenced this pull request Mar 1, 2024
I made a few small mistakes in #4, causing the tests to break. This
fixes it.

Blocked by #5.
"build": "rm -rf ./dist && rollup --config ./rollup.config.mjs",
"clean": "rm -rf ./{dist,node_modules}",
"lint": "yarn lint:eslint && yarn lint:misc --check && yarn lint:dependencies --check && yarn lint:changelog",
"lint:changelog": "auto-changelog validate --prettier",
Copy link

@legobeat legobeat Mar 2, 2024

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm aware: #6 (comment)

Will fix this in a PR soon, and also update the changelog.

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