Side effecting `Web.Fetch.Headers` module breaks build on nodejs
Closed this issue · 24 comments
Thanks a lot for creating this lib!
In my current project I've started to migrate from purescript-milkis to purescript-fetch. Unfortunately currently some modules Web.Fetch.Header module from this library are seems to be side effecting on import by using fetch API underneath. This breaks my builds on the backend even though I'm guarding against any direct usage of this lib on nodejs. I just to need to share some types between my frontend and backend.
I'm not sure but probably just moving library type declarations (like Headers) to the common, non side effecting module should fix the issue in my case.
Could you please tell me if things like this side effecting module level declaration are the final design choices and I should not expect any change in this regard?
That doesn't look like a side effecting module level declaration to me - I may be missing something but I don't think that unsafePerformEffect should be invoked until you actually call that function with an f (Tuple String String).
I don't believe there are any side-effecting module declarations. Can you clarify how your build is breaking?
I think that Headers.empty calls Headers.fromFoldable directly and Headers.fromFoldable calls Headers.unsafeNew which is a new Headers() under the hood. That I believe is causing the problem on nodejs. What do you think?
In other words I probably pointed to the wrong place as a possible source of my problem - sorry about that ;-)
Ah right yes, I see. In that case, the options I see are to have empty take a Unit argument or remove it altogether, neither of which are very appealing unfortunately.
Or have empty be a value provided by the FFI?
I think to keep the same interface we'd have to use null/undefined for empty, but I'm not sure how that affects everything else.
is there a workaround for this? I'd love to be able to use this as an interface to the api for cloudflare workers. @hdgarrood, is the objection to empty taking a Unit argument aesthetics or something else? (Apologies for potentially silly questions, I'm pretty new to purescript.)
The objection is that it would be a breaking change.
On a different note, why is unsafeNew :: Effect Headers side-effectful? Because it's not referentially transparent?
Making it take unit would not solve the problem because defaultOptions is a top-level module declaration also. Unfortunately, I think using this library in non-browser contexts will always be problematic. I don't see how we can guarantee that it will always work in other contexts. For example, what happens when you try to use the header interface for real? You would have to be very careful to only use it in specific ways, and I just don't see how we can retain being fairly exact bindings and portable. Other backends just don't support the whole API.
I just don't see how we can retain being fairly exact bindings and portable. Other backends just don't support the whole API.
Right. I think we start drawing a sharper line between the web backend, which is what this library is for, and other backends like Node.
We discussed this PR in the ES modules working group today. We concluded that this can't really be solved when other environments do not provide a full implementation of fetch. Trying to use a web library on a non-web environment unsurprisingly causes problems. Trying to support such environments at the cost of those who want to use this in a web library doesn't make sense to me.
Finally, if someone really needs this code to work, there is a workaround. They can modify the global scope and then enter the application via an asychronous function call. For example, this is what I did to get purescript-web-dom-xpath tests to pass:
- Before: https://github.com/purescript-web/purescript-web-dom-xpath/blob/c15b7402aa3293af9b2f2323b67b48c99647aa1e/test/node.js
- After: https://github.com/working-group-purescript-es/purescript-web-dom-xpath/blob/dfb82741068477e21fa66129a573f018ad30e209/test/node.mjs
Adjusting the application's entry point like that requires work that is outside of PureScript tooling.
So, we will no longer consider this issue for the 0.15.0 updates.
Do you think we could close this then?
Yes.
I'm not trying to use fetch on the backend. As I've written above just having a single project which contains import from this lib (I have it guarded in my Frontend part) breaks the build on the backend side because import is side effecting. Making this part of the API lazy would solve the issue.
Can you go into more detail about how it is breaking? What part of your build requires the code to run?
I think the problem with making the API lazy is not just that it would be a breaking change here, but that it would suggest that we should make the same change in any other web library, which could then lead to either a much larger cascade of breaking changes, or an awkward situation where we are applying different rules to different libraries based on how easy it is to make them importable under node.
If you are importing it on the backend transitively, then you are using it. I think the most robust solution is to organize your imports so you don’t transitively import inappropriate modules for your backend, rather than ad hoc changes to specific libraries. I personally don’t consider strict name resolution a side effect. As I said, it’s not just one binding, it infects anything that may reference empty at the top level. This is a strong implication that you are expecting behavior outside the semantics of the language.
Another way to put it, would it be reasonable to change purescript-node-fs because I want to reference a type that it exports in my front end build, but transitively importing it throws an error? It’s not really any different in my view.
I have isomorphic react (basic) components (server side rendering + hydration on the fronend) and I use web APIs (purescript-web) extensively all over the place. Only this lib makes problems when I want to render components on the server side because it actually calls a particular web API when imported.
But maybe you are right and I should somehow reorganize my code base and abstract away somehow fetch action... I've tried to avoid this indirection but it was probably just plain wrong to blame the lib and interpret its behavior as "side effecting" - I somewhat made just wrong assumption here ;-)
Anyway - thanks for the lib and for your time!
As @JordanMartinez noted, you can also shim the global so it doesn’t throw.
I've tried to avoid this indirection but it was probably just plain wrong to blame the lib and interpret its behavior as "side effecting" - I somewhat made just wrong assumption here ;-)
FWIW, I think the implementation of this module is definitely dodgy. The stuff in it is "unsafe" only because it's referential due to internal mutation. A more appropriate implementation (if I wanted to keep the internal mutation) would be to type it as ST instead of Effect, but then unsafePerformEffect would just be replaced by runST, so nothing would really change. I think I did it this way just because the pure interface is the only thing exported, and so I didn't see the need to bother at the time. It's certainly confusing. As I said above, one would have to choose a completely different representation of empty headers that doesn't actually invoke the Headers constructor. It's possible, but it's still just a coincidence. The actual semantics you want would be pervasive lazy top-level module bindings.
I think I did it this way just because the pure interface is the only thing exported, and so I didn't see the need to bother at the time.
Makes sens :-)
I said above, one would have to choose a completely different representation of empty headers that doesn't actually invoke the Headers constructor. It's possible, but it's still just a coincidence.
Yeah, my assumption was just plain wrong. "Lib can not load properly when its runtime dependencies are broken" seems like more reasonable one ;-)
The actual semantics you want would be pervasive lazy top-level module bindings.
Probably this occasional node.globals monkey patch as @JordanMartinez suggested would be more than enough when I'm back working on this side project ;-)