fix: serialisation nextjs requests #953
Conversation
🦋 Changeset detectedLatest commit: 56d4751 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Test coverage
|
| public baseUrl: string; | ||
| public operations: UnknownOperation[]; | ||
|
|
||
| constructor({ name, baseUrl, operations }: { name: string; baseUrl: string; operations: UnknownOperation[] }) { |
There was a problem hiding this comment.
The parameter should be interface Module?
We can use Required or something similar to not use the class and derive a type
There was a problem hiding this comment.
Do you suggest something like this?
interface ModuleDescriptor {
name: string;
baseUrl: string;
operations: UnknownOperation[];
}
class Module {
public constructor(private readonly descriptor: ModuleDescriptor) {
}I think this is better than the current approach. The current approach wastes the memory only for a better readability.
There was a problem hiding this comment.
No, I just wanted to type the constructor input
type ModuleType = Required<Module>;
class Module {
name: string;
baseUrl: string;
operations: UnknownOperation[];
public constructor({ name, baseUrl, operations }: ModuleType) {
this.name = name;
this.baseUrl = baseUrl;
this.operations = operations;
}
}
That approach wouldn't be vulnerable to this?:
const descriptor = { name: 'aModule', baseUrl: 'https://myurl.com', operations: [] };
const module = new Module(descriptor);
descriptor.baseUrl = 'https://someOtherUrlThatIsNotReadonly';
// And now the module has a different baseUrl because the `readonly` only affects `descriptor`
I would prefer an object to not depend anymore on the things it received in its constructor if possible
There was a problem hiding this comment.
No, I just wanted to type the constructor input
I don't like this one. I cannot add a public property, without influencing on the constructor arguments. I think this breaks the SRP principle.
That approach wouldn't be vulnerable to this?:
You may add the readonly keyword to any interface.
interface ModuleDescriptor {
readonly name: string;
readonly baseUrl: string;
readonly operations: UnknownOperation[];
}I would prefer an object to not depend anymore on the things it received in its constructor if possible
I agree, so you need to clearly describe which property is mutable or immutable. This is why we should use the readonly keyword for classes and interfaces.
|
@b4rtaz @FedericoAmura I continued on fixing another bug, included it in this PR as it relates to the previous discussion. This change allow us to remove the number of arguments to 2, so I removed the object interface now. I do agree with the suggestion @FedericoAmura to have a typed interface before implementing (but it is not needed anymore now). See commit cf2a8e3 for changes related to the other bug. It is about that we do not account for Nullable/Paginated resolvers. Now we resolve the operations based on the EvmApi/SolApi/Auth definitions so that we have a single source of truth for the resolving logic. |
| @@ -50,7 +54,8 @@ const MoralisNextApi = ({ authentication, ...config }: MoralisNextApiParams) => | |||
| Moralis.start(config); | |||
| } | |||
|
|
|||
| return async (req: NextApiRequest, res: NextApiResponse) => MoralisNextHandler({ req, res, authentication }); | |||
| return async (req: NextApiRequest, res: NextApiResponse) => | |||
| MoralisNextHandler({ req, res, authentication, core: Moralis.Core }); | |||
There was a problem hiding this comment.
Why do we pass here the "core" argument? The Moralis instance is the same across the whole application, so we can import it into any part of the application instead of passing it as an argument. Especially when we use this instance in the same file. Or is there something I don't get? @ErnoW
There was a problem hiding this comment.
It's a better design pattern to include these dependencies as argument (see Dependency injection)
In this case it does not make too much of a difference as in NextJs we also use directly imported instances of the 'core', like you say. But it's a good habit to include it as a dependency wherever we can. But it results in more predictable code.
This is also why we pass around the core instance within all the modules/functions within the SDK.
name: 'Pull request'
about: A new pull request
New Pull Request
Checklist
Issue Description
JSONs get double serialised before sending it to the api without deserialising the request
Related issue: #
FILL_THIS_OUTSolution Description