Add Request and Response to lib-portal #9742 #9745
Conversation
Codecov ReportBase: 84.11% // Head: 84.11% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #9745 +/- ##
============================================
- Coverage 84.11% 84.11% -0.01%
+ Complexity 19418 19417 -1
============================================
Files 2595 2595
Lines 68699 68699
Branches 5540 5540
============================================
- Hits 57787 57785 -2
- Misses 8151 8152 +1
- Partials 2761 2762 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
I think it might not be a good idea to add type parameters for Params, Cookies and Headers. The reason is that we usually can't use that same interface in both ends of the "communication", and we don't really know what shape will come in on these parameters, because the initiator of the call can really pass in anything.
| sameSite?: 'lax' | 'strict' | 'none' | ''; // '' | ||
| } | ||
|
|
||
| interface Request<Params extends Record<string, string | undefined> = Record<string, string | undefined>, |
There was a problem hiding this comment.
If you have a url with the same query parameter listed more then once, the value of the param in XP is an Array<string>
| interface Request<Params extends Record<string, string | undefined> = Record<string, string | undefined>, | |
| interface Request<Params extends Record<string, string | Array<string> | undefined> = Record<string, string | Array<string> | undefined>, |
There was a problem hiding this comment.
I think it's better to not pass in these type parameters, because your get or post function has to be able to handle any input, both missing and string and Arrays, so there is little value in locking in a shape.
The value of specifying these shapes in type parameters, would be if you could use the same shape "in both ends". That means that you specify an interface that can be used both by Request and serviceUrl() or pageUrl().
edloidas
force-pushed
the
issue-9742
branch
from
0b382f3
to
021f1f6
Compare
| contentType: string; | ||
| headers: Record<string, string | undefined>; | ||
| cookies: Record<string, string | Cookie | undefined>; | ||
| redirect: string; |
There was a problem hiding this comment.
| redirect: string; | |
| redirect?: string; |
redirect is a "write only" property, initially it is undefined
| bodyBegin?: string | string[]; | ||
| bodyEnd?: string | string[]; | ||
| }; | ||
| applyFilters: boolean; |
There was a problem hiding this comment.
optional webSocket field is missing
https://developer.enonic.com/docs/xp/stable/framework/websocket#http-websockets
| } | ||
|
|
||
| interface AssetUrlHandler { | ||
| createUrl(value: object): string; | ||
| createUrl(value: ScriptValue): string; |
There was a problem hiding this comment.
Is this depending on the global ScriptValue to exist? Because I had problems using nodeLib because of that. Can ScriptValue be imported from core or similar?
There was a problem hiding this comment.
Aren't global always exist?
__ is always there, so does __.toScriptValue. same as require
There was a problem hiding this comment.
No, not during runtime, unless global is included as dependency.
Right, I'd better revert it back to object. Totally forgot about this problem.
|
|
||
| export interface Request<Params extends RequestParams = RequestParams, | ||
| Headers extends Record<string, string | undefined> = Record<string, string | undefined>, | ||
| Cookies extends Record<string, string | Cookie | undefined> = Record<string, string | Cookie | undefined>, |
There was a problem hiding this comment.
Is it correct that a cookie can be string | Cookie | undefined? Since this comes from XP, it seems like a risky thing to get a cookie be both string and Cookie?
Could it be that cookies in Response has this shape, but not in Request?
There was a problem hiding this comment.
Request cookies are always string. (except for the cases when there is no such cookie)
There was a problem hiding this comment.
I see, let's make it string/undefined then. But the Response cookie can still be a string/Cookie/undefined, right?
| contentType?: string; | ||
| } | ||
|
|
||
| export interface Response<ResponseBody = unknown> { |
There was a problem hiding this comment.
Every field in Response should be optional, right?
There was a problem hiding this comment.
We have a dilemma here. Response that controllers receive has many non-optional fields. But the one that controllers could return may have all fields optional (because it is sort-of patch to existing one)
There was a problem hiding this comment.
It's probably necessary to have two Response types. Maybe something like this:
export interface InputResponse { // sorry about the bad naming here
status: number;
...
}
export type Response = Partial<InputResponse>;Be careful about naming it Response or Request, I think there will normally be a naming collision against the global fields in the web platform. (That was why I namespaced it in enonic-types)
| @@ -241,20 +296,20 @@ interface PageUrlHandler { | |||
| * | |||
| * @returns {string} The generated URL. | |||
| */ | |||
| export function pageUrl(params: PageUrlParams): string { | |||
| export function pageUrl<Params extends RequestParams = RequestParams>(params: PageUrlParams<Params>): string { | |||
There was a problem hiding this comment.
For all of these functions (pageUrl, serviceUrl) both number and boolean is valid types of the value (and object are just url-encoded, but that might be more coincidental).
Should maybe Params not extend RequestParams but another shape instead that allows for other types of the values.
|
|
||
| export type RequestParams = Record<string, string | string[] | undefined>; | ||
|
|
||
| export interface Request<Params extends RequestParams = RequestParams, |
There was a problem hiding this comment.
Did you consider removing (all or some of) the type parameters like I suggested earlier?
| Headers extends Record<string, string | undefined> = Record<string, string | undefined>, | ||
| Cookies extends Record<string, string | Cookie | undefined> = Record<string, string | Cookie | undefined>, | ||
| > { | ||
| method: LiteralUnion<'GET' | 'PUT' | 'POST' | 'DELETE' | 'HEAD' | 'PATCH' | 'OPTIONS' | 'TRACE' | 'CONNECT'>; |
There was a problem hiding this comment.
This is actually a list of HTTP verbs you can use in the Java-part of the system:
After discussion with @rymsha, it was decided, that
RequestandResponsetypes will go tolib-portal, because it's where they actually belong in Java as part of theportal-api.