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

fix: bump shared eslint config version #565

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

adamstankiewicz
Copy link
Member

@adamstankiewicz adamstankiewicz commented Jun 20, 2024

The shared @edx/eslint-config was recently updated to support the es2020 environment such that syntax like globalThis won't cause linting errors.

This PR bumps the version of @edx/eslint-config to pull in this change for consuming MFEs and shared JS libraries (e.g., frontend-platform).

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Looks great! I tested this in frontend-platform and frontend-app-course-authoring. After this bump, it stopped showing globalThis as an eslint error.

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Jun 20, 2024

BTW, something I mentioned to @davidjoy recently: I don't understand why we even have eslint-config as a separate repo. It contains 16 boilerplate files and 1 useful .eslintrc.js file which could just as easily be merged into this repo's config/.eslintrc.js file. Plus, eslint-config and frontend-build specify different versions of eslint and various dependencies.

And according to npm, it's only used by frontend-build anyways.

@adamstankiewicz
Copy link
Member Author

BTW, something I mentioned to @davidjoy recently: I don't understand why we even have eslint-config as a separate repo. It contains 16 boilerplate files and 1 useful .eslintrc.js file which could just as easily be merged into this repo's config/.eslintrc.js file.

And according to npm, it's only used by frontend-build anyways.

edx/eslint-config predates @openedx/frontend-build by awhile. @edx/eslint-config is still consumed by other frontend-facing repos (e.g., edx-platform, studio-frontend, credentials, Paragon), which don't rely on @edx/frontend-build.

The ESLint config in @openedx/frontend-build specifically is intended only for MFEs and more recent shared JS libraries that consume it. That said, it does seem like there's a few non-legacy repos still referring to @edx/eslint-config when they should be relying solely the base config from @edx/frontend-build (example).

Plus, eslint-config and frontend-build specify different versions of eslint and various dependencies.

According to the peerDependencies in @edx/eslint-config, it supports both v7 and v8 of ESLint; @openedx/frontend-build relies on v8 of ESLint. As long as the peerDependencies can be fulfilled, I'm not sure there's much concern around slightly different ESLint versions between these two packages, so long as they are compatible with each other.

@adamstankiewicz adamstankiewicz enabled auto-merge (squash) June 21, 2024 11:22
@adamstankiewicz adamstankiewicz merged commit 4af4a57 into master Jun 21, 2024
5 checks passed
@adamstankiewicz adamstankiewicz deleted the ags/bump-eslint-config branch June 21, 2024 13:03
@openedx-semantic-release-bot

🎉 This PR is included in version 14.0.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 15.0.0-alpha.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bradenmacdonald
Copy link
Contributor

I see now. Thanks for the context on that, @adamstankiewicz !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants