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

Vite middleware support: Request type missing export and .originalUrl method & url setting #249

Closed
DadiBit opened this issue Apr 11, 2024 · 7 comments

Comments

@DadiBit
Copy link
Contributor

DadiBit commented Apr 11, 2024

Greetings, I wanted to use vite together with hyper-express and tsx for a full API+backend developing (production ready) server.
Unluckily, the vite server (mis?)uses the req object and writes to the url property + it also uses the originalUrl property.

  1. I tried importing Request and extending it. Typescript is not complaining while building, however the library doesn't export the type, not sure why.
  2. Vite uses the originalUrl property, which is defined only in the types, however after executing the compiled code it complains that Cannot set property originalUrl of #<Request> which has only a getter
  3. It also sets the url property, not sure if it's bad practice (I think so, but anyway).

Workarounds:

  1. Idk
  2. Define an originalUrl in the source Request.js class
  3. Define a simple set url(value) { this._url = value; } in the source Request.js class

Solutions:

  1. Make the Request class properly exported and thus, extendable, and let user deal with point 2 and 3 in a custom extended class.
@kartikk221
Copy link
Owner

Interesting, this seems to be some sort of undocumented legacy usage of a property from Express.js

Both the Request and Response types are exported here https://github.com/kartikk221/hyper-express/blob/f5111f84510a93990e56146417fa29d05eebd14e/types/index.d.ts#L4C1-L5C44

Do you want to make a PR which resolves this or should I push a minor version update?

I believe I implemented originalUrl as just a setter which would explain your error, so a simple setter should do the trick.
Also, I do think setting the url property is a bad practice but some legacy libraries / implementations just didn't care.
Regardless, a setter for the url property should also do the trick.

@DadiBit
Copy link
Contributor Author

DadiBit commented Apr 12, 2024

Interesting, this seems to be some sort of undocumented legacy usage of a property from Express.js

Both the Request and Response types are exported here https://github.com/kartikk221/hyper-express/blob/f5111f84510a93990e56146417fa29d05eebd14e/types/index.d.ts#L4C1-L5C44

Yep, I saw them, but I don't really know what happened with the exports. The VS code linter doesn't complain (even links to the .d.ts type), yet tsc creates a "class MyRequest extends undefined", which leads to an error at runtime.

Do you want to make a PR which resolves this or should I push a minor version update?

I can do it, but I have no idea how to fix the import/export issue.

I believe I implemented originalUrl as just a setter which would explain your error, so a simple setter should do the trick. Also, I do think setting the url property is a bad practice but some legacy libraries / implementations just didn't care. Regardless, a setter for the url property should also do the trick.

Ok, I'm making a PR with those setters asap.

@DadiBit
Copy link
Contributor Author

DadiBit commented Apr 12, 2024

Actually, I just realized there are discrepancies between the type definitions/exports and the js source files.

Exports

The exported type definitions:

export * as compressors from 'uWebSockets.js';
export * from './components/Server';
export * from './components/router/Router';
export * from './components/http/Request';
export * from './components/http/Response';
export * from './components/middleware/MiddlewareHandler';
export * from './components/middleware/MiddlewareNext';
export * from './components/plugins/LiveFile'
export * from './components/plugins/MultipartField'
export * from './components/plugins/SSEventStream'
export * from './components/ws/Websocket'

The JavaScript exports:

hyper-express/index.js

Lines 16 to 20 in f5111f8

module.exports = {
Server,
Router,
compressors: uWebsockets,
};

So typescript thinks that the types are available, but the underlying source code is missing (since the actual javascript doesn't export any Request or anything).

Properties

Types definitions:

/* ExpressJS Properties */
locals: Locals;
protocol: string;
secure: boolean;
ips: string[];
subdomains: string[];
hostname: string;
fresh: boolean;
stale: boolean;
xhr: boolean;
body: any;
params: ParamsDictionary;
query: ParsedQs;
originalUrl: string;
baseUrl: string;

Exported JavaScript class:
Missing some express.js properties (in particular originalUrl and baseUrl - please note that they're used just in routers)

Missing ExpressRequest and ExpressResponse type declarations

Self explainatory, see: /src/components/compatibility

The main issue for the discrepancies is not having a single source of truth (ie, typescript .ts files) - to be fair: you need to be a madman to port everything in full typescript .ts source code, so I'm not asking for this "better solution"

How would you like me to fix the Request class @kartikk221 ?
Also do you want me to fix the export thing?

@kartikk221
Copy link
Owner

Wait, I am confused. Shouldn't TypeScript be using the index.d.ts as the truth for its resolving? The JSDoc types and Typescript types are "loosely" similar because certain precision can be achieved in TypeScript which can't be achieved in JSDoc hence the differences.

@DadiBit
Copy link
Contributor Author

DadiBit commented Apr 12, 2024

I was also confused (I never worked with .d.ts), in theory I think .d.ts files can also avoid using the same folder structure as the javascript source code, thus the compiler won't actually look for the exports as if it was a mirror.

Easy peasy solution with the exports in index.js:

// ... imports
const Request = require('./src/components/http/Request.js');
const Response = require('./src/components/http/Response.js');

// Disable the uWebsockets.js version header if not specified to be kept
// ...

module.exports = {
    Server,
    Router,
    Request,
    Response,
    compressors: uWebsockets,
};

Same goes for any other javascript classes that were exported in the .d.ts
PS I edited the previous comment to fix one link and explain a bit better for future readers.

@DadiBit
Copy link
Contributor Author

DadiBit commented Apr 12, 2024

Exporting Request should also fix the extends undefined issue from the tsc compiled JavaScript code, letting the final user extend it and implement any missing method/property to use vite.

Also, I saw there's an ExpressRequest/Response javscript class, but no type definition, is it still experimental or is it ready --> should I work on the .d.ts types?

@kartikk221
Copy link
Owner

If exporting on the JS side solves the issue then I guess I don't see the issue. I just don't understand why the JS has to line up with .d.ts types since it defeats the purpose of the .d.ts types.

The types for ExpressRequest are not documented and need to be documented / extended through the .d.ts types.

DadiBit added a commit to DadiBit/hyper-express that referenced this issue Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants