Closed eringram closed 2 months ago
anmolshres98
commented
2 months ago I'm not sure if checking for the repo url is the best way to identify if a repo is from the core repo or not. I can't think of a better alternative yet but let's think about it some more before concluding with checking on the url. Did you try and use any other approaches?
eringram
commented
2 months ago I'm not sure if checking for the repo url is the best way to identify if a repo is from the core repo or not. I can't think of a better alternative yet but let's think about it some more before concluding with checking on the url. Did you try and use any other approaches?
Yeah it doesn't feel the best to check the url string like this, but I couldn't come up with a better way either. Checking for certain package names doesn't work unless we maintain a list of the allowed packages, which could be more annoying and prone to becoming outdated. Maybe the new field approach like arun mentioned above.
anmolshres98
commented
2 months ago I'm not sure if checking for the repo url is the best way to identify if a repo is from the core repo or not. I can't think of a better alternative yet but let's think about it some more before concluding with checking on the url. Did you try and use any other approaches?
Yeah it doesn't feel the best to check the url string like this, but I couldn't come up with a better way either. Checking for certain package names doesn't work unless we maintain a list of the allowed packages, which could be more annoying and prone to becoming outdated. Maybe the new field approach like arun mentioned above.
hmmm, I don't like the new field approach either. With the checks we are thinking of right now, from a security point of view any pkg can circumvent our rule check by changing values in their repo since these values aren't validated at all. Imo for now, if this is something we are trying to get out asap it is fine but we should think of a better solution.
What about checking for common parent in file tree?
anmolshres98
commented
2 months ago @eringram , we had a conversation within the core team yesterday and we concluded that this is a bug on studio's codebase and not on the eslint-plugin side. This change would be introducing complexity/bug to fix a bug on studio.
From studio's perspective, they are linting node_modules of studio apps to make sure that dependencies of studio apps are not using internal apis. This is ok however, this also checks itwinjs-core packages for internal api usage. itwinjs-core packages do not depend on any other bentley packages and is barebones + these packages are allowed to use internal apis from other packages inside the itwinjs-core repo. Because of this rationale, itwinjs-core packages should be trusted to not be using internal apis and hence studio should not be linting itwinjs-core packages at all. In fact, studio code already has an exclusion list of packages that are not under the bentley/itwin purview: https://github.com/iTwin/studio/blob/b00a62291e4286b453e96c6b0172e3f57defb0ae/packages/apis/create-itwin-studio-app/ts-worker.ts#L140C1-L143C53
The correct fix here would be to add itwinjs-core packages to that exclusion list so that they are not longer linted and the erroneous reports would go away.
Lmk if you that is something you would be interested in working on in the studio codebase otherwise I would delegate the task to myself or put it on studio's backlog. Either way this PR can be closed.
eringram
commented
2 months ago Because of this rationale, itwinjs-core packages should be trusted to not be using internal apis and hence studio should not be linting itwinjs-core packages at all. In fact, studio code already has an exclusion list of packages that are not under the bentley/itwin purview: https://github.com/iTwin/studio/blob/b00a62291e4286b453e96c6b0172e3f57defb0ae/packages/apis/create-itwin-studio-app/ts-worker.ts#L140C1-L143C53
The correct fix here would be to add itwinjs-core packages to that exclusion list so that they are not longer linted and the erroneous reports would go away.
@anmolshres98 This explanation makes sense, thanks for discussing it with the team. One question I still have is how this applies to Booster++, where the issue also came up - see this comment about erroneous violations. It seems like they are also linting node_modules for a similar reason of enforcing their dependencies to not use internal APIs, so I guess the solution for them would also be to exclude itwinjs-core packages however they set up linting?
Lmk if you that is something you would be interested in working on in the studio codebase otherwise I would delegate the task to myself or put it on studio's backlog. Either way this PR can be closed.
I have other issues to work on for the display team and am not familiar with studio, so I think someone else would be better suited to work on this. I can close this PR but might leave the branch for future reference.
aruniverse
commented
2 months ago One question I still have is how this applies to Booster++
Does booster not use the linting tools provided by Studio, therefore if we update Studio's linting scripts, Booster and other Studio Apps would pick this up for automatically?
eringram
commented
2 months ago Does booster not use the linting tools provided by Studio, therefore if we update Studio's linting scripts, Booster and other Studio Apps would pick this up for automatically?
I wasn't aware of how they are related, so if booster uses those same tools then you're right it sounds like it'll be fixed
This PR allows internal APIs within the itwinjs-core repository to be used by any other package in that repository. This is done by comparing the package.json repository.url of the file being linted and the file containing the violating internal API, and allowing the internal usage if both have the url https://github.com/iTwin/itwinjs-core.git. The package.json files are located using the findup-sync package.
Additional changes:
tagoptioncontext.parserServicestocontext.sourceCode.parserServicesbecause the former is deprecated and will be removed in ESLint v9