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

Packages ts-invariant and subscriptions-transport-ws both have reference to @types/node #7734

Closed
jimisaacs opened this issue Feb 19, 2021 · 12 comments
Assignees

Comments

@jimisaacs
Copy link

jimisaacs commented Feb 19, 2021

Please refer to this issue for context: microsoft/TypeScript#31148 (comment)

In summary, the older versions of the ts-invariant and subscriptions-transport-ws dependencies are exposing a reference to @types/node, which leaks and pollutes typescript codebases. They seem to have fixed their packages though in their sources.

After further search, the versions of the packages in question is not the issue, but the issue remains with them regarding the original typescript issue.

@jimisaacs jimisaacs changed the title Please upgrade ts-invariant and Please upgrade ts-invariant and subscriptions-transport-ws Feb 19, 2021
@benjamn
Copy link
Member

benjamn commented Feb 19, 2021

@jimisaacs What version of @apollo/client are you using? The current latest version of @apollo/client (3.3.11) is using the latest version of ts-invariant (0.6.0) and subscriptions-transport-ws (^0.9.17, matching 0.9.18), unless I'm missing something.

@jimisaacs
Copy link
Author

I see we have 3.3.11 installed, let me do some more digging on the other packages.

@jimisaacs
Copy link
Author

jimisaacs commented Feb 19, 2021

For some reason we have ts-invariant 0.4.4, not sure about that yet. I'm assuming something else is happening their in my monorepo. We have subscriptions-transport-ws 0.9.18, so I don't understand. If you download from npm, that version, and look at dist/server.d.ts, you should see /// <reference types="node" /> right at the top. Though I don't see it in the source here https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/server.ts#L1 so 🤷

I will look again at why we have that version of ts-variant, though subscriptions-transport-ws has me stumped at the moment.

@jimisaacs
Copy link
Author

So, I can't find out why 0.4.4 is installed from ts-invariant, but I guess it doesn't matter, as it is in the same state as the other package. The reference is there in the published declarations, but I didn't find it in the source.

@jimisaacs
Copy link
Author

Given the versions of the packages are not the issue, but that apollo-client is the source of both of these packages in my codebase, I'm going to retitle this issue, but keep it open.

@jimisaacs jimisaacs changed the title Please upgrade ts-invariant and subscriptions-transport-ws Packages ts-invariant and subscriptions-transport-ws both have reference to @types/node Feb 19, 2021
@Venryx
Copy link

Venryx commented Feb 20, 2021

I have the same issue. It's apparently stemming from the processStub variable in ts-invariant/src/invariant.ts.

When compiled to the lib folder, TypeScript apparently resolves the variable's type to NodeJS.Process, as seen below:

And I can't get rid of ts-invariant as a dependency, because apparently it is a subdependency of @apollo/client -- which means my browser-targeted apollo library is now requiring the NodeJS types. 🤦‍♂️

@Venryx
Copy link

Venryx commented Feb 20, 2021

I think the reason the processStub variable is being resolved to NodeJS.process during compilation is due to the following line, in the package.json: https://github.com/apollographql/invariant-packages/blob/7813ca014a3ad5110850d8640063766fa58f24f7/tsconfig.json#L9

"types": ["node", "mocha"],

Because TypeScript has the NodeJS types included, when it's compiling, I think it notices that the type of window.process is set to NodeJS.Process, as seen here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/52df09eae2c3892c9338d25b3d9080ca73c97ef6/types/node/v13/globals.d.ts#L148

declare var process: NodeJS.Process;

Temp solution (in userland)

Manually remove the /// <reference types="node" /> line, and change the typing of processStub to any.

Long-term solution

I'm guessing it can be fixed by removing the "types": ["node", "mocha"], line from the package.json in the root of the invariant-packages repo.

@jimisaacs
Copy link
Author

@Venryx So you are saying tsc actually adds the /// <reference types="node" /> to the top of the generated declarations automatically, because there is "types": ["node"] in the tsconfig file used to generate said declaration?

@Venryx
Copy link

Venryx commented Feb 22, 2021

So you are saying tsc actually adds the /// to the top of the generated declarations automatically, because there is "types": ["node"] in the tsconfig file used to generate said declaration?

I'm no expert on the details of TypeScript's compilation process, but that's what it looks like to me, yes.

Well more precisely, I think it adds that reference line because the processStub variable is "resolved" to the NodeJS.Process type defined from the @types/node package (because of the const processStub = globalThis.process line) -- which could only happen because of that "types": ["node"] line being present.

benjamn added a commit to apollographql/invariant-packages that referenced this issue Feb 23, 2021
Should resolve #74 and the ts-invariant side of
apollographql/apollo-client#7734.

Added a quick test:no-node script to fail if the word "node" (any
capitalization) ever creeps into ts-invariant/lib/invariant.d.ts again.
benjamn added a commit to apollographql/invariant-packages that referenced this issue Feb 23, 2021
Should resolve #74 and the ts-invariant side of
apollographql/apollo-client#7734.

Added a quick test:no-node script to fail if the word "node" (any
capitalization) ever creeps into ts-invariant/lib/invariant.d.ts again.
benjamn added a commit that referenced this issue Feb 23, 2021
@benjamn
Copy link
Member

benjamn commented Feb 23, 2021

@jimisaacs @Venryx PR #7751 should fix the ts-invariant part of the problem, but you can also update to ts-invariant@0.6.1 on your own, in the meantime.

@benjamn benjamn self-assigned this Feb 23, 2021
benjamn added a commit that referenced this issue Feb 23, 2021
Should fix the `ts-invariant` part of issue #7734, thanks to
apollographql/invariant-packages#81.
@jimisaacs
Copy link
Author

Much appreciated @benjamn, and @Venryx for the thorough analysis.

@hwillson
Copy link
Member

Fixed by #7751 - thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants