itwinjs-core
itwinjs-core copied to clipboard
Break out of lockstep versioning for tooling packages
Can we break locktstep for these tooling pkgs like certa?
Originally posted by @aruniverse in https://github.com/iTwin/auth-clients/pull/81#discussion_r940433404
This should help us be able to more quickly react to security fixes too.
I'd like to add build-tools and eslint-plugin to the list -- e.g. dropping support for old eslint versions or fixing vulnerabilities in build scripts shouldn't require a semver-major bump in all our packages.
Dropping my findings that I already discussed with Arun and Paulius.
I am trying to use @bentley/react-scripts@5 with @itwin/[email protected] but it seems that they are incompatible since eact-scripts has eslint v8 while @itwin/eslint-plugin still uses everything from eslint v7 therefore I am getting different errors when building our project. For testing I tried to update all the packages in @itwin/eslint-plugin to the latest versions and it seemed to fix everything.
@iTwin/itwinjs-core-ui thoughts on killing off the itwin/eslint-plugin ui rule from core in favor of one from itwinui? cc @iTwin/itwinui
@aruniverse not sure what you mean. itwinui has no eslint plugin
I see you guys have an internal one, wonder f we should publish that if we opt to remove the core-ui one.
@aruniverse I'm not sure what you call "ui" rule, if it is the "react-set-state-usage.js" stuff, I think this could be killed as this is for Class component, which we are moving away from anyway.
I see you guys have an internal one, wonder f we should publish that if we opt to remove the core-ui one.
You mean an eslint config ? I dont see the benefit of "publishing" that, it's really up to the dev teams to use whatever they see fit. (And given the number of eslint-ignore we have, not sure the rule is that much of a sharable one.) And I'm not convinced switching to using another team config would help much, except pop errors... That is, unless "Bentley" as a whole agree on what these rules should be, which I doubt will ever be the case.
Here is the itwinjs-ui specific eslint config published by core, https://github.com/iTwin/itwinjs-core/blob/master/tools/eslint-plugin/dist/configs/ui.js Which is whats being pushed both internally/externally.
I guess i just mainly want to make sure we need this still it, that it make senses today (rules are up to date), and to find actual owners to maintain this config
@NancyMcCallB can chime it, maybe there is a reason to publish this in the first place, but I think this config should be moved to appui and only be used locally, by our team, in our repo, and not pretend to instruct other devs the "rules" they should follow. I don't feel it is our business to mantain and validate external dev behavior at this level. (It does make sense for us to have a proper rule config though, and the one there might need some review, I havent looked at this)
I think it was published in the first place because the others were published. Unless we have a strong philosophical reason to control the rules for external devs, I agree with @raplemie .
These configs are meant to serve as a starting point for other devs and to provide some amount of consistency between Bentley products. Of course everyone is free to add their own rules or disable some of the default ones if they wish so, but at least they don't have to construct their whole configs from scratch.
As a general note, though, I think we should revise our main recommended config too because I know quite a few lines in it don't change anything from @typescript-eslint/recommended so we should remove them. In the long term, I think we should just move to Prettier as some teams have already done. That provides the style consistency we want but we don't have to maintain a config with ever changing rules (they change on major version bumps of either eslint or one of the plugins). The eslint plugin would stay to enforce best practices and warn about bugs such as leaving floating promises behind.
Speaking with @paulius-valiunas, a good first step is to move eslint-plugin out of the core repo: #5080 - @yashincontrol and I will handle it.