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

Upgrade ESLint and Babel. Use Wren Security ESLint conventions. #165

Merged
merged 4 commits into from
Feb 15, 2024

Conversation

krausvo1
Copy link
Contributor

This PR adds our ESLint conventions (@wrensecurity/eslint-config) to the project. ESLint, Babel and related Grunt dependencies have been upgraded to the latest versions.

React specific ESLint rules come from ForgeRock's former forgerock/react shared config. Those could be moved to a separate shared config in the future.

The ESLint rules I have deleted are already present in @wrensecurity/eslint-config.

I have tested both admin and user UI, everything works fine. Production build runs without any errors. Built code sync in development mode also works as expected.

@pavelhoral
Copy link
Member

On a second thought all of non-ES5 features have to be dropped because of the overhead that the transpilation will introduce (helper functions in every). We can switch to ES2022+ when we drop RequireJS (as r.js understands only ES5). This will be hopefully done soon, but not right now.

@krausvo1
Copy link
Contributor Author

There is already lot of non-ES5 features in current code (and the code is currently being transpiled). Do you think we should get rid of these already present occurences? If so, I think we could do it in a separate PR and keep this PR aimed at adopting new shared ESLint configuration.

I have dropped non-ES5 changes my changes have introduced. Most of the changes are fixes of ESLint errors related to formatting.

@pavelhoral
Copy link
Member

There is already lot of non-ES5 features in current code (and the code is currently being transpiled). Do you think we should get rid of these already present occurences? If so, I think we could do it in a separate PR and keep this PR aimed at adopting new shared ESLint configuration.

I have dropped non-ES5 changes my changes have introduced. Most of the changes are fixes of ESLint errors related to formatting.

I am OK with sending all code through some sort of limited transpilation (or even the full one) if it does not generate additional unwanted code for ES5-only files.

@krausvo1
Copy link
Contributor Author

I see. Would it be OK to handle that in a separate PR?

@krausvo1
Copy link
Contributor Author

I have added our shared ESLint React config, added function parameters related rules (inspired by Wren:IDM rules) and dropped copyright headers updates from files with only trivial whitespace changes.

@krausvo1 krausvo1 force-pushed the upgrade-ui-eslint branch 3 times, most recently from 553499c to 0fef4d2 Compare February 15, 2024 13:49
Copy link
Member

@pavelhoral pavelhoral left a comment

Choose a reason for hiding this comment

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

LGTM, ty.

@pavelhoral pavelhoral merged commit 13c0244 into WrenSecurity:main Feb 15, 2024
2 checks passed
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.

2 participants