Add node types to container-loader

[ChumpChief]

We aren't getting type checking on node functionality we're using in the container-loader package (e.g. url.parse). This includes @types/node and does fixup for a timer type usage that was assuming window.setTimeout.

[vladsud]

I feel like this goes against @curtisman changes. I think the desire is to use tslib signatures, that have number here.

[ChumpChief]

Maybe the problem is unique to parse, I'll look around for if a more targeted fix exists. I'd like to understand better though if this is a safe direction to go (e.g. what if I'm running this on Node rather than in the browser, where it is in fact not a number).

[ChumpChief]

More targeted fix in #3145. This PR isn't really needed anymore re: parse type and could be closed out, though I'm wondering if the implicit usage of window is something we should be concerned about.

[vladsud]

I agree with you that using number or timer looks wrong, as it's correct only on one target. The right fix here is to use isomorphic wrapper with its own type, I assume. I do not know if we are getting anything by not using Node types, I hope Curtis can chime in.

[curtisman]

@heliocliu have a fix where the type is derived from the 'ReturnType' of the API, so it is correct in both context.
he returned value is abstract to us anyways, so in reality any time is fine.

We shouldn't include node types to make sure these isomorphic packages don't accidentally depends on things that needs polyfill or stop working for browser (which has happened before)

[heliocliu]

That's the same change Matt makes here, minus adding the Node typings. Probably still makes sense to add the ReturnType change though, such as for tests that need the typings

[ChumpChief]

I generally agree with trying to avoid polyfill usage where possible, but I don't believe excluding the types prevents that. Here we're already depending on the browserify url polyfill in utils.ts -- we just don't get type info for it.

Other options considered:

1. Explicitly include the url package in package.json -- doesn't change anything
2. Look for isolated typing of the url package -- doesn't seem to be an option, only included as part of @node/types here.
3. Switch to standard URL, avoiding the polyfill and also getting typings -- tried in Use URL instead of parse in container-loader utils #3145, but learned the standard URL doesn't handle unknown protocols the same way. We should probably get rid of our usage of non-standard/unknown protocols but this would be a non-trivial change.

If adding node types is a non-starter then I'll probably just close both PRs out and log bugs for (1) removing our usage of non-standard protocols (2) converting usage of parse() to URL().

[ChumpChief]

Closing this out. Opened #3187 and #3188 to track the direction of removing unknown protocols and using standard URL