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

Elysia 1.0 Breaking Changes / Migration #513

Closed
SaltyAom opened this issue Mar 1, 2024 · 18 comments
Closed

Elysia 1.0 Breaking Changes / Migration #513

SaltyAom opened this issue Mar 1, 2024 · 18 comments
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers pending Should be resolved in next release

Comments

@SaltyAom
Copy link
Member

SaltyAom commented Mar 1, 2024

Breaking change

It's sad but the upcoming Elysia 1.0 RC will contain breaking changes for all users, especially library authors :aris_crying:

Lifecycle event register with "on" will now only apply to the current and descendant instances.

Migration

Migration should take no longer than 10-15 minutes.
But for production apps, it heavily depends on library authors.

Starting from Elysia 1.0 RC, all lifecycle events including derive, and resolve will accept additional parameters { as: 'global' | 'local' } to specify if hook should be local or global which default to local

To migrate, add { as: 'global' } to any even register using .on to specify it as global.

// From Elysia 0.8
new Elysia()
    .onBeforeHandle(() => "A")
    .derive(() => {})

// Into Elysia 1.0
new Elysia()
    .onBeforeHandle({ as: 'global' }, () => "A")
    .derive({ as: 'global' }, () => {})

Behavior

If { as: 'global' } is not added, a parent instance that uses the current instance will not inherits a hook as the following:

const plugin = new Elysia()
    .onBeforeHandle(() => {
        console.log('Hi')
    })
    // log Hi
    .get('/hi', () => 'h')

const app = new Elysia()
    .use(plugin)
    // no log
    .get('/no-hi', () => 'h')

If is a behavior change from 0.8.

  • 0.8: Both /hi and /no-hi will log
  • 1.0: Only /hi will log

As the plugin hook now only defaults to itself and its descendant, it will not be applied to the parent (main).

To make it apply to the parent, explicit annotation is needed.

const plugin = new Elysia()
    .onBeforeHandle({ as: 'global' }, () => {
        console.log('Hi')
    })
    // log Hi
    .get('/hi', () => 'h')

const app = new Elysia()
    .use(plugin)
    // no log
    .get('/no-hi', () => 'h')

This will mark a hook as global, which is a default behavior of 0.8.

API Affected

To help with this, you can usually find a global hook by using find all with .on.
For derive, mapDerive, resolve, mapResolve will show a type warning as missing if use outside of the scope.

The following API or need to be migrate:

  • parse
  • transform
  • beforeHandle
  • afterHandle
  • mapResponse
  • onError
  • onResponse
  • trace
  • derive
  • mapDerive
  • resolve
  • mapResolve

API that does NOT need to be migrated:

  • request

Why

We know we don't like migration as well, but we think this is an important breaking change that will happen sooner or later to fix current problems:

  • We found that many developers have a lot of nested guard even on the new instance. Guard is now almost used as a way to start a new instance that will not affect the main one. To fix this, we default the hook to local by default, removing the boilerplate need.
  • We believe that every hook global by default can cause unpredictable behavior if not careful, especially in a team with inexperienced developers. We asked many developers and found that many expected the hook to be local at first without reading the documentation.
  • Following the previous point, we found that making hook global by default can easily cause accidental bugs if not reviewed carefully, and hard to debug as of observability is hard to distinguish by eyes especially in large PR and codebase. We want to reduce the said user-error as much as possible.

Once migrated, your API should have an easier mental model, be more predictable, and be easier to maintain.
For example, see the below picture which does the same thing.

Screenshot 2567-03-01 at 19 48 07

For additional information, please see
https://x.com/saltyAom/status/1763554701130514944?s=20

@SaltyAom SaltyAom added good first issue Good for newcomers pending Should be resolved in next release labels Mar 1, 2024
@SaltyAom SaltyAom self-assigned this Mar 1, 2024
@SaltyAom SaltyAom pinned this issue Mar 1, 2024
@cybercoder-naj
Copy link

Hi! Is there any place I can contribute to updating the docs for v1?

@dodas
Copy link

dodas commented Mar 15, 2024

How is this supposed to work with Plugins?
Consider this example:

import { Elysia } from "elysia";

const myPlugin = () => (a: Elysia) =>
  a.derive(() => ({ myPluginProp: 1 }));

const app = new Elysia()
  .use(myPlugin())
  .get("/my-plugin", ({ myPluginProp }) => {
//                      ^ Property 'myPluginProp' does not exist on type '{ body: unknown; ...
    return myPluginProp
  });

It seems the app actually gets myPluginProp on runtime (expected), just TS types say otherwise.

@kabir-asani
Copy link

Returning an object in v1.0.3 is not converting to response to application/json type. Rather, it sends it out as a text/plain. How do I fix this?

@dodas
Copy link

dodas commented Mar 18, 2024

@SaltyAom any thoughts on this. Seems like this completely prevents usage of plugins using derive in v1.

@SaltyAom
Copy link
Member Author

@dodas update to elysia 1.0.6

kravetsone added a commit to kravetsone/elysia-oauth2 that referenced this issue Mar 19, 2024
@PureDizzi
Copy link

PureDizzi commented Mar 20, 2024

Returning an object in v1.0.3 is not converting to response to application/json type. Rather, it sends it out as a text/plain. How do I fix this?

@kabir-asani A quick workaround I found is to spread the object into another object.

e.g.
return { ...someObject }

@kabir-asani
Copy link

@PureDizzi Thank you!
This seems to be working but it's ugly.

We need a fix on this!

@april83c
Copy link

april83c commented Mar 26, 2024

Returning an object in v1.0.3 is not converting to response to application/json type. Rather, it sends it out as a text/plain. How do I fix this?

@kabir-asani A quick workaround I found is to spread the object into another object.

e.g. return { ...someObject }

For correct typing in the Eden client, return { ...new Example() } as Example seems to work (this bug is so weird... has anyone made an issue for it yet?)

@SaltyAom
Copy link
Member Author

Returning an object in v1.0.3 is not converting to response to application/json type. Rather, it sends it out as a text/plain. How do I fix this?

@kabir-asani A quick workaround I found is to spread the object into another object.

e.g. return { ...someObject }

For correct typing in the Eden client, return { ...new Example() } as Example seems to work (this bug is so weird... has anyone made an issue for it yet?)

I want to get this fix.
Can you provide some example that cause this problem?

@kabir-asani
Copy link

import { Elysia } from "elysia";

class Data {
    message: String;

    constructor(message: String) {
        this.message = message;
    }
}

const app = new Elysia()
    .get("/ping", ({ set }) => {
        const data = new Data("pong");

        set.status = 200;

        return data;
    })
    .listen(3000);

@SaltyAom
This snippet is enough to replicate the issue -- please check.

NOTE: What I've noticed is that the issue arises only when I try to access the set object.

@SaltyAom
Copy link
Member Author

SaltyAom commented Apr 2, 2024

import { Elysia } from "elysia";

class Data {
    message: String;

    constructor(message: String) {
        this.message = message;
    }
}

const app = new Elysia()
    .get("/ping", ({ set }) => {
        const data = new Data("pong");

        set.status = 200;

        return data;
    })
    .listen(3000);

@SaltyAom This snippet is enough to replicate the issue -- please check.

NOTE: What I've noticed is that the issue arises only when I try to access the set object.

Hi, sorry for the slow response.

Unfortunately, this is an expected behavior and the case without using set is a bug.

Web Standard on different runtime has a slight implementation for mapping value to Response.

Some classes may expected to pass to Response directly while some classes are not on different implementation, instead of serializing all the unknown case, we leave this gray area for users to handle themself using either mapResponse or defining a serialization method using toString

In this case, if you are using a custom class, you may use toString method like this instead.

class Data {
	message: String

	constructor(message: String) {
		this.message = message
	}

	toString() {
		return JSON.stringify(this)
	}
}

@kabir-asani
Copy link

But is it really a fool-proof solution?
Because then I'd have to set Content-Type explicitly as well.

@dodas
Copy link

dodas commented Apr 3, 2024

I propose this:

  • have users define toJSON() method on their data classes, which returns the plain, json-serializable representation of the object
  • have Elysia call toJSON() method if it's available and if so, respond with that along with Content-Type: application/json

@binyamin
Copy link

binyamin commented Apr 7, 2024

@SaltyAom I understand that you don't want to introduce breaking changes after v1. Have you seen #99 (comment)? I am biased, but that seems like an easy thing to slip in with other breaking changes.

@bogeychan
Copy link
Contributor

@april83c, @kabir-asani, you can do something like this as a workaround:

const Data = class Object {
  message: String;

  constructor(message: String) {
    this.message = message;
  }
};

return new Data("yay") // inside handler

The problem is this switch on constructor name:

switch (response?.constructor?.name) {

@bogeychan
Copy link
Contributor

or implement toJSON like this:

class Data {
  message: String;

  constructor(message: String) {
    this.message = message;
  }

  toJSON() {
    return structuredClone(this);
  }
}

That way you dont have to set Content-Type:

new Elysia()
  .get("/", () => {
    return new Data("yay").toJSON();
  })
  .listen(8080);

Its the same behaviour as return { "message": "yay" }

@dilraj-vidyard
Copy link

I am really not a fan of the { as: 'global' }. It makes the code so cluttered and unfamiliar...

Also, I think mention the { as: type } in the At A First Glance section would be helpful! It's buried all the way the bottom in the Scope section.

@kravetsone
Copy link
Contributor

I am really not a fan of the { as: 'global' }. It makes the code so cluttered and unfamiliar...

Also, I think mention the { as: type } in the At A First Glance section would be helpful! It's buried all the way the bottom in the Scope section.

Since 1.1 you can use .as("global") on elysia instance

gtramontina pushed a commit to gtramontina/elysia-htmx that referenced this issue Jul 27, 2024
Elysia 1.0 introduces a breaking change for life cycle events.

> Starting from Elysia 1.0 RC, all lifecycle events including derive,
> and resolve will accept additional parameters
> { as: 'global' | 'local' } to specify if hook should be local or
> global which default to local

elysiajs/elysia#513

elysia-htmx 1.0.9 does not specify such an option, and so any use of the
plugin results in the `hx` property not being typed as available in the
parent Elysia instance's handlers.

This commit updates dependencies to Elysia 1.0.10 and adds the as:
"global" option to allow the hx parameter to be available in parent
Elysia instances.

This mirrors how the official Bearer token plugin does it:
https://github.com/elysiajs/elysia-bearer/blob/main/src/index.ts#L56

Signed-off-by: Snorre Magnus Davøen <[email protected]>
gtramontina pushed a commit to gtramontina/elysia-htmx that referenced this issue Jul 27, 2024
Elysia 1.0 introduces a breaking change for life cycle events.

> Starting from Elysia 1.0 RC, all lifecycle events including derive,
> and resolve will accept additional parameters
> { as: 'global' | 'local' } to specify if hook should be local or
> global which default to local

elysiajs/elysia#513

elysia-htmx 1.0.9 does not specify such an option, and so any use of the
plugin results in the `hx` property not being typed as available in the
parent Elysia instance's handlers.

This commit updates dependencies to Elysia 1.0.10 and adds the as:
"global" option to allow the hx parameter to be available in parent
Elysia instances.

This mirrors how the official Bearer token plugin does it:
https://github.com/elysiajs/elysia-bearer/blob/main/src/index.ts#L56

Signed-off-by: Snorre Magnus Davøen <[email protected]>
@SaltyAom SaltyAom added the documentation Improvements or additions to documentation label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers pending Should be resolved in next release
Projects
None yet
Development

No branches or pull requests

10 participants