-
Notifications
You must be signed in to change notification settings - Fork 17
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
Instrumenting fetching algorithm parts #48
Comments
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. |
I'm fine with it being punted, just recording and pointing that this needs to be worked on eventually. |
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, 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. |
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 |
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. |
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. |
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). |
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 ( Say we add back the 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. |
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.
The text was updated successfully, but these errors were encountered: