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

Rework routes/plugins #55

Merged
merged 14 commits into from
Feb 15, 2024
Merged

Rework routes/plugins #55

merged 14 commits into from
Feb 15, 2024

Conversation

inetol
Copy link
Contributor

@inetol inetol commented Feb 13, 2024

Este PR reimplementa todas las rutas y plugins a un sistema mejor organizado y compatible para futuras novedades. Una de ellas es la posibilidad de minificar y compilar a un binario standalone, este cambio permitirá imágenes de contenedor y ejecuciones en crudo considerablemente más compactas, rápidas y potencialmente más seguras.

Ejec. crudo:
Antes 74,3 MB
Ahora 269,8 KB (Solo con el index.js) (No se tiene en cuenta el runtime de Bun)

Imágenes:
Antes 173.1 MB (2024.02.09-339e4ba)
Ahora 98.1 MB (424e791)

@inetol inetol self-assigned this Feb 13, 2024
@inetol inetol linked an issue Feb 13, 2024 that may be closed by this pull request
@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

@tnfAngel Como crees que sería la mejor forma de manejar las rutas? Ahora mismo el #group() actúa añadiendo un prefijo al path (es la teoría, pero el problema reside en que si dejo todos los path como estaban originalmente, las rutas aparecen en la raíz del dominio y no en /api/v1/documents), lo he quitado temporalmente hasta buscar otra forma de hacerlo.

Para los alias he pensado en inicializar las rutas en un Array y luego registrar cada ruta como se está haciendo ahora en la rama "dev" pero no sé si hay métodos mejores.

@inetol inetol linked an issue Feb 14, 2024 that may be closed by this pull request
@tnfAngel
Copy link
Member

Ahora mismo el #group() actúa añadiendo un prefijo al path (es la teoría, pero el problema reside en que si dejo todos los path como estaban originalmente, las rutas aparecen en la raíz del dominio y no en /api/v1/documents), lo he quitado temporalmente hasta buscar otra forma de hacerlo.

Por qué aparecen en la raíz si se usa el group precisamente para evitar eso?

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Por qué aparecen en la raíz si se usa el group precisamente para evitar eso?

Hasta donde yo he podido llegar a ver, como estoy definiendo los path con la ruta completa, al añadir el #group() se le añade el prefijo y queda algo como por ejemplo "/documents/api/v2/documents" en el caso de index de la V2 donde "/documents" sería el group y "/api/v2/documents" la ruta que se registra en la clase.

@tnfAngel
Copy link
Member

Por qué aparecen en la raíz si se usa el group precisamente para evitar eso?

Hasta donde yo he podido llegar a ver, como estoy definiendo los path con la ruta completa, al añadir el #group() se le añade el prefijo y queda algo como por ejemplo "/documents/api/v2/documents" en el caso de index de la V2 donde "/documents" sería el group y "/api/v2/documents" la ruta que se registra en "la ruta".

Ahh, y no se puede de alguna forma remover la parte de /documents/api/v2 inicial (con un split a / o replace) y en el group poner el /documents/api/v2 en cuestión?

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Ahh, y no se puede de alguna forma remover la parte de /documents/api/v2 inicial (con un split a / o replace) y en el group poner el /documents/api/v2 en cuestión?

No sé si Elysia permite sobreescribir o eliminar rutas, si registro ":key" se podrá acceder por http://localhost:4000/:key (y http://localhost:4000/api/v1/documents/:key si hago group), por eso me veo obligado a registrar la ruta completa para evitar este problema (al menos con el método que he implementado)

@tnfAngel
Copy link
Member

Ahh, y no se puede de alguna forma remover la parte de /documents/api/v2 inicial (con un split a / o replace) y en el group poner el /documents/api/v2 en cuestión?

No sé si Elysia permite sobreescribir o eliminar rutas, si registro ":key" se podrá acceder por http://localhost:4000/:key (y http://localhost:4000/api/v1/documents/:key si hago group), por eso me veo obligado a registrar la ruta completa para evitar este problema (al menos con el método que he implementado)

Entonces simplemente podemos prescindir del group y registrarla de forma "absoluta" no? tampoco hay mucho problema, el scope de plugins por ejemplo será general de todas formas, no por grupos

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Entonces simplemente podemos prescindir del group y registrarla de forma "absoluta" no? tampoco hay mucho problema, el scope de plugins por ejemplo será general de todas formas, no por grupos

¿Pero entonces como se registra "/documents" para la V2? Se itera mil veces para registrar cada alias que se quiera meter? No sé las repercusiones de esto si Elysia cargara todo lo que haya en #register() varias veces

@tnfAngel
Copy link
Member

tnfAngel commented Feb 14, 2024

Entonces simplemente podemos prescindir del group y registrarla de forma "absoluta" no? tampoco hay mucho problema, el scope de plugins por ejemplo será general de todas formas, no por grupos

¿Pero entonces como se registra "/documents" para la V2? Se itera mil veces para registrar cada alias que se quiera meter? No sé las repercusiones de esto si Elysia estará cargando todo lo que haya en #register()

Para los alias se registra la V2 y los alias a la vez (comprobando obviamente que la versión es la última), en la misma iteración, a no ser que el sistema de alias se haga más "genérico" y registrarlo a parte, en vez de usarlo solo para este caso, y register solo se deberia llamar 1 vez por ruta no?

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Para los alias se registra la V2 y los alias a la vez (comprobando obviamente que la versión es la última), en la misma iteración, y register solo se deberia llamar 1 vez por ruta no?

Se puede llamar #register() todas las veces que quieras, lo que me preocupa es si eso va a ir llenando considerablemente la clase principal de Elysia con el mismo código una y otra vez. Voy a probar a ver si no aparecen problemas.

@tnfAngel
Copy link
Member

tnfAngel commented Feb 14, 2024

Para los alias se registra la V2 y los alias a la vez (comprobando obviamente que la versión es la última), en la misma iteración, y register solo se deberia llamar 1 vez por ruta no?

Se puede llamar #register() todas las veces que quieras, lo que me preocupa es si eso va a ir llenando considerablemente la clase principal de Elysia con el mismo código una y otra vez. Voy a probar a ver si no aparecen problemas.

Creo que tendría el mismo efecto/impacto que usando grupos, al final el group es solo una forma de agregarle prefijo y scope, pero todas las rutas se guardan en el Array interno Elysia.routes tenga grupo o no

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Parece funcionar correctamente de esta forma y sin hacer apaños cutres

@inetol
Copy link
Contributor Author

inetol commented Feb 14, 2024

Ya de paso que lo tengo fácil para probar el tope de rutas que aguanta una clase de Elysia, he creado 350 endpoints y lo único que ha petado ha sido mi navegador al abrir los docs así que esta implementación es muy viable.

En total, con esos 350 endpoints llega a 84 MB de memoria, por lo que no será un problema añadir tantos alias como haga falta.

@inetol inetol changed the title Rework routes Rework routes/plugins Feb 15, 2024
@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

@tnfAngel El plugin de ErrorSender lleva ya unos cuantos commits que no actualiza los types de Elysia cuando lo cargas.

@tnfAngel
Copy link
Member

@tnfAngel El plugin de ErrorSender lleva ya unos cuantos commits que no actualiza los types de Elysia cuando lo cargas.

probablemente por el override del return type de servidor elysia al cargar el plugin, ya que esos plugins registran genericos en el tipo, y en el override se devuelve el tipo sin genericos

@tnfAngel
Copy link
Member

voy a ver

@tnfAngel
Copy link
Member

tnfAngel commented Feb 15, 2024

(ignora la guarrada que hice en el cargador de plugins, es de prueba), es eso por lo que veo

Quitandole el return type para que lo infiera con genéricos

imagen
imagen
imagen

Con return type explícito

imagen
imagen

@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

(ignora la guarrada que hice en el cargador de plugins, es de prueba), es eso por lo que veo

Quitandole el return type para que lo infiera con genéricos

Con return type explícito

No ya, pero y en las rutas? No puedo dejar el constructor de AbstractRoute sin un type específico

@tnfAngel
Copy link
Member

(ignora la guarrada que hice en el cargador de plugins, es de prueba), es eso por lo que veo
Quitandole el return type para que lo infiera con genéricos
Con return type explícito

No ya, pero y en las rutas? No puedo dejar el constructor de AbstractRoute sin un type específico

ese es el problema.. hmmm

@tnfAngel
Copy link
Member

quedaría muy feo hardcodear Elysia<"", {
request: {};
store: {};
derive: {
errorSender: ErrorSender;
};
resolve: {};
}, {
type: {};
error: {};
}, {}, {}, {}, false>

@tnfAngel
Copy link
Member

tnfAngel commented Feb 15, 2024

exportando el typeof server e importandolo en la clase AbstractRoute ?

@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

exportando el typeof server e importandolo en la clase AbstractRoute ?

No funciona

@tnfAngel
Copy link
Member

exportando el typeof server e importandolo en la clase AbstractRoute ?

No funciona

diría de registrar los plugins necesarios que se usen en la ruta en la propia ruta, pero eso impediría el tema de solo tener una instancia

@tnfAngel
Copy link
Member

aunque elysia hace deduplicacion de plugins si se nombran, pero no se que tal seria eso....

@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

aunque elysia hace deduplicacion de plugins si se nombran, pero no se que tal seria eso....

El ErrorSender no se podría instanciar en DocumentHandler y así ya no involucraría a las rutas?

@tnfAngel
Copy link
Member

aunque elysia hace deduplicacion de plugins si se nombran, pero no se que tal seria eso....

El ErrorSender no se podría instanciar en DocumentHandler y así ya no involucraría a las rutas?

claro, pero no se que tan buena idea sea instanciarlo en cada request, aunque haya deduplicacion

@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

claro, pero no se que tan buena idea sea instanciarlo en cada request, aunque haya deduplicacion

De momento lo dejaré así, por suerte solo son los types y a Bun se la pela compilar el código con este fallo.

@inetol inetol marked this pull request as ready for review February 15, 2024 21:42
@inetol inetol requested a review from tnfAngel February 15, 2024 21:42
@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

Curioso, en la documentación se puede ver el orden en que los endpoints han sido registrados

@tnfAngel
Copy link
Member

tnfAngel commented Feb 15, 2024

sip, y creo que seria mejor que la última versión sea la primera siempre (por eso el .toReversed())

@inetol
Copy link
Contributor Author

inetol commented Feb 15, 2024

sip, y creo que seria mejor que la última versión sea la primera siempre

Ya está puesto, digo que cada path se ordena de forma manual, en este caso cuando se registran. Si comparas https://jspaste.eu/docs con la de este branch se puede apreciar que ahora están por orden alfabético (depende de como lo pongas en el Array obvio no hay ningun sorter), cosa que antes no pasaba y era aleatorio (bc magia del .scanSync)

Screenshot from 2024-02-15 23-02-31 Screenshot from 2024-02-15 23-02-41

Copy link
Member

@tnfAngel tnfAngel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

package.json Show resolved Hide resolved
src/classes/Server.ts Show resolved Hide resolved
@inetol inetol merged commit 1cbb77e into dev Feb 15, 2024
4 checks passed
@inetol inetol deleted the rework-routes branch February 15, 2024 22:17
inetol added a commit that referenced this pull request Feb 15, 2024
@inetol inetol mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework routes SIGTERM failed to stop container in 10 seconds, resorting to SIGKILL
2 participants