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

Use @definitelytyped/types-registry for ATA #33791

Closed
wants to merge 5 commits into from

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Oct 3, 2019

Use @definitelytyped/types-registry for ATA and fall back to types-registry if npm install @definitelytyped/types-registry doesn't work.

Notes:

  1. This currently requires you to be authenticated with npm. After the Github package registry leaves beta, that will no longer be true. This PR should not be merged until GHPR is generally available, but it's useful for testing until that date.
  2. The existing error handling is incorrect -- it handles an exception that's already handled, while ignoring the return code of execSyncAndLog. I corrected it so that it logs if either npm or GHPR fails.
  3. I haven't added tests for this change yet. I see some references to types-registry in the tests so I'll see if that's the right place.

Step 2 of #33330

Edit: Ignoring whitespace makes the diff a little easier to read.
Further edit: I had to create a new file, so the diff is basically impossible to read. You're better off looking at just the first commit to figure out what the PR does, then reading the rest separately.

Fall back to types-registry if `npm install
@definitelytyped/types-registry` doesn't work.

Notes:

1. This currently requires you to be authenticated with npm. After the
Github package registry leaves beta, that will no longer be true.
2. The error handling was incorrect -- it double-handled an exception and
ignored the return code of execSyncAndLog. I corrected it so that it
logs if either npm or GHPR fails.
3. I haven't added tests for this change yet. I see some references to
types-registry in the tests so I'll see if that's the right place.
@sandersn
Copy link
Member Author

sandersn commented Oct 3, 2019

Adding some reviewers for visibility mainly.

The tests all mock the registry itself and override the constructor. (This explains why TypingsInstaller is an apparently superfluous base-class -- it's all the pure code in one place, with NodeTypingsInstaller doing all the mutation.)

Any idea how this could be tested?

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

For testability, you could move the execSync implementation onto this.installTypingsHost?

src/typingsInstaller/nodeTypingsInstaller.ts Outdated Show resolved Hide resolved
@sandersn
Copy link
Member Author

So. The second commit is now unit tested. This was the result of lots of flailing about, so let me explain what happened so nobody has to repeat it.

  1. NodeTypingsInstaller is now in typingsInstallerCore, which is the "library" part of typingsInstaller. No instances are created as a side-effect of including typingsInstallerCore in your compilation. That is not true of typingsInstaller. I don't like the typingsInstaller/typingsInstallerCore alternation. I'm considering [-App/-Core] instead, plus a comment at the top of -App explaining that it is intentionally not unit-testable.
  2. NodeTypingsInstaller now uses undefined instead of "UPDATE FAILED" as an error marker.
  3. execSyncAndLog now returns true for success instead of true for failure. This makes the whole calling tree simpler to read.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Meant to come back around and approve this after the testing changes 👍

@sandersn
Copy link
Member Author

Since this is reviewed, I'm going to merge as-is when GPR comes out of beta, but at our meeting on Friday @minestarks pointed out that we should work with the editor to have it pass a --registry= flag to typingsInstaller so that in case of emergency it can be disabled quickly by shipping an editor update, not a typescript update.

@minestarks
Copy link
Member

minestarks commented Nov 11, 2019

@sandersn Thanks -- the ability to configure / turn off the registry override is a pretty important requirement from my perspective, can we track it with an issue to make sure it doesn't slip through the cracks by 3.8 beta?

@sandersn
Copy link
Member Author

#35038

@sandersn
Copy link
Member Author

sandersn commented Dec 5, 2019

This is blocked until Github Packages allows installation of packages without authenticating with a github user's credentials, which it does not yet. I'll update the bug when that does. In the meantime, I may fold in the fix to #35038 to this PR.

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@sandersn
Copy link
Member Author

This may not be needed since presumably npm and github packages will soon be sharing infrastructure and microsoft will be paying the bill for hosting both.

@sandersn sandersn added Experiment A fork with an experimental idea which might not make it into master and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 17, 2020
@sandersn
Copy link
Member Author

sandersn commented Feb 2, 2022

After discussing this, we decided that this feature doesn't make sense with Microsoft owning both github and npm. There's no gain in redundancy anymore.

@sandersn sandersn closed this Feb 2, 2022
@jakebailey jakebailey deleted the use-definitelytyped-for-ata-types-registry branch November 7, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Experiment A fork with an experimental idea which might not make it into master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants