loaders icon indicating copy to clipboard operation
loaders copied to clipboard

Instrumenting fetching algorithm parts

Open bmeck opened this issue 4 years ago • 10 comments

the thread of https://twitter.com/bradleymeck/status/1454093832539320325 pointed out with the consolidation of hooks that the provided during the fetching of a resource body from a URL no longer is automated by the nature of the hooks.

this somewhat mirrors Yarn (CC: @arcanis ) and its desire to have node automate parts of the URL operations for things.

Likely we should investigate how to instrument this. Likely 2 sets of hooks that are available one for instrumenting the resolver in the node algorithm and one for the actual fetching operations.

bmeck avatar Oct 29 '21 14:10 bmeck

The reason we consolidated the hooks was to enable chaining. I think we need to land chaining first before considering any new hooks, because for any new hooks the question will become how to preserve chaining with the addition of the new hooks.

GeoffreyBooth avatar Oct 30 '21 01:10 GeoffreyBooth

I'm fine with it being punted, just recording and pointing that this needs to be worked on eventually.

bmeck avatar Oct 30 '21 01:10 bmeck

Yeah absolutely. I think for now the solution that I feel confident about is utility functions. If you want to propose new hooks that still can work with our current design for chaining, that can work too.

GeoffreyBooth avatar Oct 30 '21 02:10 GeoffreyBooth

@GeoffreyBooth, I am not confident that your idea about chaining is solid, and it still has not been presented, so please try not to make your proposed chaining a constraint for future developments on loaders and the module system in general. There seem to be quite a few things that we have overlooked, seeing as how we still don't have fetch in core, and that is actually very relevant, but I digress.

DerekNonGeneric avatar Oct 30 '21 03:10 DerekNonGeneric

The plan for chaining was presented and discussed in this week's meeting, and the design doc is merged in and linked from this repo's readme. We've been discussing chaining all year; it was the reason @JakobJingleheimer spent months on the consolidation PR.

I don't see any issues presented in that Twitter thread. It was known that moving getSource and transformSource within load would mean more boilerplate for folks writing load hooks. I was the one who created transformSource for just that reason, to avoid boilerplate. But chaining is a higher priority, and we'll have to find other ways to avoid boilerplate. Utility functions are one way, and if there's a way to do more hooks without breaking the use cases people want chaining for, then that would be fine too.

GeoffreyBooth avatar Oct 30 '21 05:10 GeoffreyBooth

[…] it was the reason @JakobJingleheimer spent months on the consolidation PR.

The reason for the consolidation was because the way we had the hooks before was illogical.

That hook consolidation also did not have my approval nor any sort of consensus-agreement w/ the team.

Just because you stream a video to youtube doesn't mean that the relevant core team members are tuned in.

DerekNonGeneric avatar Oct 30 '21 05:10 DerekNonGeneric

  • https://www.w3.org/TR/webarch/#def-coneg
  • https://www.w3.org/2001/tag/issues.html#contentTypeOverride-24

DerekNonGeneric avatar Oct 30 '21 06:10 DerekNonGeneric

FYI: the start of the tweet that @bmeck linked to was mine. In the end, it was a bug on my part and not a problem in the loader interface. I found that I could get rid of the code that read the file explicitly.

giltayar avatar Oct 30 '21 07:10 giltayar

Fwiw, I find it difficult to weight on the chaining design if fs hooks aren't included. The util approach @GeoffreyBooth has in mind doesn't strike me as convincing (I mentioned it at least here), but if it's out of scope for these discussions then I'm worried it will be too late later (the hook consolidation is already used as a supporting reason why fs hooks would be best avoided, even if perhaps they'd have been considered if discussed in the previous iteration).

arcanis avatar Oct 30 '21 07:10 arcanis

The design docs are at https://github.com/nodejs/loaders/blob/main/doc/design/overview.md. I think the way forward is for proposals for new hooks to be PRs to add docs here. We've already started this in some of the open PRs. There's a design doc there for chaining, so any new proposed hooks for chaining would need to explain how they'd support chaining along the lines of how the existing hooks will.

If I remember correctly, the issue with many hooks for chaining was that each return value that Node uses (format, source) needs to have only one hook that returns that value. Otherwise it's not clear how to order the loaders.

Say we add back the transformSource hook and it returns source too; how will chaining work between that hook and load? All the load hooks run, then all the transformSource hooks? But we might need transpilation to happen before the final load hook is run, because of some other loader that does something in load that needs to be last, and now we have a transpilation loader not using the transformSource hook that was designed for it.

These are the types of issues that chaining introduces. They're not unsolvable, but this is why I think we need to land chaining first. We can design other hooks now, but their designs should include how they'd address these issues.

GeoffreyBooth avatar Oct 30 '21 17:10 GeoffreyBooth