Skip to content
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

Update CONTRIBUTION.MD #71

Open
kbventures opened this issue Jan 14, 2025 · 1 comment · May be fixed by #76
Open

Update CONTRIBUTION.MD #71

kbventures opened this issue Jan 14, 2025 · 1 comment · May be fixed by #76
Assignees

Comments

@kbventures
Copy link
Contributor

kbventures commented Jan 14, 2025

Add these points:

  • Favoring function declaration when possible
  • Utilize relative imports
  • Error handling(The only part of the code that returns a response to the network is the request handler, every other function either returns or throws an error.)
@madcampos
Copy link
Collaborator

So, here is a breakdown of the points and the reasoning for them...

Relative imports

Imports on the web only recognise urls. "Bare imports" is a thing that started with node and then it evolved into import maps.

URL imports basically come down to those 3 ways of referencing an import:

  • A relative import that start with ./ or ../;
  • An absolute import that starts with /, and points to the base domain;
  • An absolute URL to a different domain, that starts with https://.

Think about it as the path to an image or css file, it is the same things.

When we do it for the project, it keeps consistency and compatibility with the web and reduces the amount of "translation" needed to reference a file.

The drawbacks are that if we move a file around it changes everything that depends on that file, and we have to update all of the imports; and that reading a file import can be "wonky" like ../../some/folder/file.ts.

The good part is that it improves discoverability of dependencies because everything is made more explicit.

So, what about import maps? They are a neat tool to help make bare imports work on the web. But they add a layer of complexity and indirection. There is no easy way to tell "where does this come from?"

As a general guideline:

  • All code that is ours should use relative imports to make it easier to reason about.
  • We should not use absolute imports (i.e. the ones starting with /) because transforms, bundling and all other things may break the paths.
  • All external dependencies should use bare imports. For those we want to keep the node style.

This helps create consistency and differentiation of internal vs external code and keep our dependencies transparent to us, it doesn't matter much where they come from in the end.

Request handling

To make the code more consistent and improve the separation of concerns, responses to the network should only be done on the request handler.

That doesn't mean that other functions or pieces of code shouldn't guide what to send down the wire, like if there is an error or not. It means that code that processes data, should not care about the request, only it's input and output.

This helps making code more testable, where all functions that handle business logic can be unit tested without worrying about the request or response parts, they only care about the data coming in and the data going out.

Error handling

As a general idea, we want to cascade and propagate errors to the outermost layer. That way, if something goes wrong, the code breaks and don't execute more than expected and the error propagates to a place that can handle it properly.

In practical terms, it means request handlers will be the place where error handling happens generally, and then other pieces of code should throw errors.

Note that subclassing of errors to add more metadata, is an interesting way to help the handling of specific types of errors.

Here is an example of subclassing:

class DataValidationError extends Error {
  statusCode: number;

  constructor(message: string, statusCode: number) {
    super(message);

    this.statusCode = statusCode;
  }
}

function dataValidation() {
  throw new DataValidationError('Invalid data', 400);
}

function requestHandler(context: HonoContext) {
  try {
    dataValidation();
  } catch (err) {
    // This is a validation error, so we have the status code available
    if (err instanceof DataValidationError) {
      return context.json({ error: err.message }, err.statusCode);
    }

    // This is a regular error, so it may be comming from somewhere else.
    // Here we treat it like a server error.
    return context.json({ error: err.message }, 500);
  }
}

An exception to this is when we want to silently ignore errors, or we want to handle things differently even if an error occurs, like adding a default return. On that case it is okay to use try..catch in other places.

Favouring function declaration over function expressions

In general, we want to prefer function declarations for a couple reasons:

As a rule of thumb:

  • If it has a name, use a regular function declaration.
  • If it is used as a "one off" anonymous function (e.g.: as an argument for Array.map), use an arrow function expression.

@madcampos madcampos linked a pull request Jan 19, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

6 participants