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

Add core-common to the list of packages for electron-auth and browser-auth to be marked as workspace:* #6360

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

nick4598
Copy link
Contributor

After running rush update --full in the release branch a couple of days ago and a failing docs build: https://dev.azure.com/bentleycs/iModelTechnologies/_build/results?buildId=2612229&view=logs&j=bf01bec3-f6e2-5983-bdff-93019feebaa1&t=8d110379-66a4-5f89-b46c-1a9b11ce4c18

I decided to debug into typedoc/typescript code and poke around. In a broken docs build (after running rush update --full), we were missing some exports from RpcManager. This was due to RpcManager now resolving to a path like "D:/itwinjs-core2/common/temp/node_modules/.pnpm/@itwin+corecommon@4.3.3_67wltvhdskk2oee2c3z2o4tfly/node_modules/@itwin/core-common/lib/cjs/RpcManager.d.ts" instead of "D:/itwinjs-core/core/common/lib/cjs/RpcManager.d.ts".

When running typedoc we intentionally exclude paths with node_modules (by passing an exclude regex) in the name which makes sense. Even when hovering over imports in vscode "import from @itwin/core-common" these incorrect paths would be shown which suggested that typescript module resolution was giving these paths to typedoc. I later confirmed that while stepping through typedoc and ending up in typescript code, but I didn't quite know why TS seemed to be getting confused.

The reason rush update --full caused this is due to the below
image
Because browser-auth was pulling in a version of @itwin/core-common... Since locally our package.json had 4.3.3 in it as well as in the pnpm-lock after rush update --full, this was confusing TS and it was no longer looking at the local copy of core-common but the installed version of core-common which was also 4.3.3. This lines up with why master works just fine, locally we had a dev version and installed we had some other officially released version. That also supports why release/4.3.x worked just fine before we had run rush update --full. Locally we had 4.3.3 and installed we had a 4.2.1 version.

Thanks Bill for noticing that we had an installed copy of core-common in the common/temp/.pnpm/node_modules and thanks Ben for general help with this.

Open Questions

  1. Its confusing to me that I only had to change browser-auth in pnpmfile.cjs to get this to work again, when electron-auth should've been introducing the same problem (Note I did change both in the PR anyways).
  2. Should this have caused more problems with docs than it did? Or does core-electron just have the perfect conditions to expose the problem. It feels like all that needs to happen is an import from core/common for it to have broken elsewhere.. but I don't know.

@nick4598 nick4598 requested a review from a team as a code owner January 11, 2024 19:43
@nick4598
Copy link
Contributor Author

@Mergifyio backport release/4.3.x

Copy link
Contributor

mergify bot commented Jan 11, 2024

backport release/4.3.x

✅ Backports have been created

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

brutal

@nick4598 nick4598 enabled auto-merge (squash) January 12, 2024 14:14
@nick4598 nick4598 merged commit b484d9b into master Jan 16, 2024
12 checks passed
@nick4598 nick4598 deleted the nick/fixauth branch January 16, 2024 22:20
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
…-auth to be marked as workspace:* (#6360)

(cherry picked from commit b484d9b)

# Conflicts:
#	common/config/rush/pnpm-lock.yaml
aruniverse pushed a commit that referenced this pull request Jan 16, 2024
aruniverse pushed a commit that referenced this pull request Jan 17, 2024
…-auth to be marked as workspace:* (backport #6360) [release/4.3.x] (#6370)

Co-authored-by: Nick Tessier <22119573+nick4598@users.noreply.github.com>
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.

3 participants