itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Break out of lockstep versioning for tooling packages

Open aruniverse opened this issue 2 years ago • 2 comments

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.

aruniverse avatar Aug 08 '22 16:08 aruniverse

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.

paulius-valiunas avatar Sep 28 '22 12:09 paulius-valiunas

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. image

bentleyvk avatar Oct 12 '22 16:10 bentleyvk

@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 avatar Dec 21 '22 18:12 aruniverse

@aruniverse not sure what you mean. itwinui has no eslint plugin

mayank99 avatar Dec 21 '22 18:12 mayank99

I see you guys have an internal one, wonder f we should publish that if we opt to remove the core-ui one.

aruniverse avatar Dec 21 '22 18:12 aruniverse

@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.

raplemie avatar Dec 21 '22 18:12 raplemie

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.

raplemie avatar Dec 21 '22 18:12 raplemie

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

aruniverse avatar Dec 21 '22 18:12 aruniverse

@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)

raplemie avatar Dec 21 '22 18:12 raplemie

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 .

NancyMcCallB avatar Dec 21 '22 20:12 NancyMcCallB

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.

paulius-valiunas avatar Dec 21 '22 21:12 paulius-valiunas

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.

ben-polinsky avatar Feb 15 '23 15:02 ben-polinsky