denoland / fresh

The next-gen web framework.
https://fresh.deno.dev
MIT License
12.15k stars 619 forks source link

Hooks stop working when importing npm modules #2390

Closed ooker777 closed 5 months ago

ooker777 commented 5 months ago
import { useState } from "preact/hooks";
import builder from "npm:utm-builder";

export default function test() {
  const [value, setFn] = useState(null); 
  const a = builder("https://example.com", "sdf", "sdf", "sdf");

  return (
    <>
      <input
        type="text"
        onInput={(e) => {
          setFn(e.target.value)}}
      /><br />
      {a}{value} 
    </>
  );
}

The npm module works fine in Deno, but in Fresh it makes the hooks stop working.

marvinhagemeister commented 5 months ago

Running the snippet you provided and opening the browser's DevTools console, we see the following error:

Screenshot 2024-03-30 at 20 41 02

Looking at the source of the utm-builder npm module on GitHub reveals that they rely on the built in url module from node .

// See  https://github.com/mahnunchik/utm-builder/blob/9feff4f343176d54bee9666f46c2ecd753cc1c4a/index.js#L3

// This "url" a module provided by Nodejs. In more modern code bases
// you'll find it under the "node:url" alias instead.
var url = require('url');

// ...
module.exports = function(link, source, medium, campaign, content, term) {
  // ...
}

Built-in modules from node are not available in the browser and cannot be easily shimmed. So this module won't work in the browser.

The module's description "utm-builder adds custom campaign parameters to your URLs." made me curious. Looking at the GitHub history it was last updated 10 years ago (excluding README + package.json updates). That puts it right after the very first public 0.10.x Nodejs release was made available to the world.

Screenshot 2024-03-30 at 21 05 02

A lot has happened in the last 10 years and URL handling has gotten much simpler. Every JavaScript runtime ships with the native URL class out of the box for example.

Using the URL constructor we can write a much simpler version of what the utm-builder module provides and make it even typesafe in the process, so that you get some nice autocompletion in your editor!

// Does the exact same as the `utm-builder` npm module. On top of that,
// this implementation allows passing URL instances directly and is type safe.
function utmBuilder(
  link: string | URL,
  source: string,
  medium: string,
  campaign: string,
  content?: string,
  term?: string
) {
  const url = new URL(link);
  url.searchParams.set("utm_source", source);
  url.searchParams.set("utm_medium", medium);
  url.searchParams.set("utm_campaign", campaign);

  if (content !== undefined) {
    url.searchParams.set("utm_content", content);
  }

  if (term !== undefined) {
    url.searchParams.set("utm_term", term);
  }

  return url;
}

tl;dr: Running built-in modules from Node in the browser is not supported. In this case the npm dependency that relies on node:url is more 10 years old and can be replaced with a simple function these days, thanks to the URL class.

ooker777 commented 5 months ago

That's wild. Thank you so much for a detail analysis on the problem. I take that when Node was just released there was no way to convert Node modules to run in the browsers. Is that correct? I'm also surprised that JS didn't have the URL class as well, since it seems to be a basic feature.

I also wonder why it stops the hooks? I know that the browsers don't support that, but it's irrelevant to them. It's similar to https://github.com/denoland/fresh/issues/2312#issuecomment-1943724237 in that the hook shouldn't be relevant to the unsupported function.

And why can't we convert the unsupported function to have it run on browsers?

marvinhagemeister commented 5 months ago

I take that when Node was just released there was no way to convert Node modules to run in the browsers.

When webpack, the first mainstream bundler, came onto the scene they tried to do just that. Their original goal was to make all code written for Node work in the browser. As part of that developers translated some easier built in modules from node for the browser. This lead to many websites amount of JavaScript increase by a factor of 4-10x and made them much slower. This confused many newer developers. Their own code was pretty small, why were the .js files shipped to the browser so insanely big? Well, turns out more than half of the code that was shipped to the browser was about shimming Node's builtin APIs. Whilst it added a nice convenience for developers writing the code, it was ultimately a nightmare for the actual users of those websites. It did more harm than good.

I'm also surprised that JS didn't have the URL class as well, since it seems to be a basic feature.

Oh you might be surprised by how little we had back then. Most of the things developers would consider essential these days simply didn't exist. It was a time where folks were still mind blown by the fact that you could even run JavaScript on the server at all. Before that JavaScript was only seen as a toy language to add some little animations or do a simple network requests in the browser. It was definitely a different time than today.

I also wonder why it stops the hooks? I know that the browsers don't support that, but it's irrelevant to them. It's similar to https://github.com/denoland/fresh/issues/2312#issuecomment-1943724237 in that the hook shouldn't be relevant to the unsupported function.

The way JavaScript works is that before any code is run at all, it needs to download the files referenced in the import statements. It then comes across the url import and realizes that it doesn't have that module and throws an error. This error is thrown before any of the JavaScript code is run. It's not like it ran everything up until the hooks and then something stopped working. What happened is that none of the JavaScript code was executed. You can try this for yourself with the following snippet:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <title>Document</title>
  </head>
  <body>
    <script type="module">
      import foo from "./does-not-exist.js";
      // This console.log will never be run
      console.log("hey");
    </script>
  </body>
</html>

If you put this snippet into an .html file and open that in your browser, you'll see that console.log("hey") is never printed to the browser's DevTools console. Reason for that is that it failed in the module loading stage, before any modules are executed. The same thing happened in your case.

And why can't we convert the unsupported function to have it run on browsers?

The built in Node API is fairly big. Making all of that run in the browser is roughly a 3-5 years of engineering effort. It's not an easy thing to do. Whilst it might have worked in this particular case as the node:url module is one of the smaller ones, it would still mean that you'd ship lots of unnecessary code to your users. That might not matter much for a personal site or something with few users. But in e-commerce for example shipping a slow site often translates directly to losing a big amount of money because impulsive buyers don't have the patience to wait for slow sites to load. Shipping unnecessary code does more harm than good in the end. With Fresh being used by a few e-commerce developers, it's better to fail early than to allow developers to unknowingly ship lots of unnecessary JavaScript to their users.

ooker777 commented 5 months ago

So does Deno encourage the use of bundler? When you say "it's better to fail early than to allow developers to unknowingly ship lots of unnecessary JavaScript to their users", I take that it doesn't. However I'm not sure how frameworks be much different on playing the role of bundler?

marvinhagemeister commented 5 months ago

You don't need a bundler for Deno, you need one for the browser. TypeScript doesn't work in the browser, JSX doesn't work in the browser, npm: and jsr: module prefixes don't work in the browser, etc. All these kind of things we paper over in Fresh by using a bundler. Deno doesn't need that because it supports these things natively. In that sense Deno doesn't encourage the use of bundlers for running code in Deno. It's encouraged to use with browsers though.

When you say "it's better to fail early than to allow developers to unknowingly ship lots of unnecessary JavaScript to their users", I take that it doesn't.

Well, it did fail for you, otherwise you wouldn't have opened this issue :-) We should print a better error message though.

However I'm not sure how frameworks be much different on playing the role of bundler?

Yeah, pretty much every framework that aims to make building websites or web apps easier uses a bundler under the hood. These days more and more framework features are implemented through the bundler, so the lines between framework and bundler are pretty blurry these days.