Maintenance: Parser getTestEvent function can be simplified

[svozza]

Summary

While working on #3301, I noticed that the getTestEvent function is doing synchronous reads, file path resolution and JSON parsing when loading in the JSON test events. While this works, it is possible to load JSON files directly in TypeScript and let the runtime take care all that housekeeping by using dynamic import:

export const getTestEvent = <T extends Record<string, unknown>>({
  eventsPath,
  filename,
}: {
  eventsPath: string;
  filename: string;
}): T =>
  JSON.parse(
    readFileSync(
      join(__dirname, '..', '..', 'events', eventsPath, `${filename}.json`),
      'utf-8'
    )
  ) as T;

// becomes

export const getTestEvent = async <T extends Record<string, unknown>>({
  eventsPath,
  filename,
}: {
  eventsPath: string;
  filename: string;
}): Promise<T> => {
  const { default: event } = await import(`../../events/${eventsPath}/${filename}.json`);
  return event as T;
};

This does mean that getTestEvent will now be an async function so we'd have to change all the tests to reflect this. Again, I'm happy to implement this if we decide to do it.

Why is this needed?

Using the built in module resolution is simpler than hand-rolling it ourselves.

Which area does this relate to?

Parser

Solution

No response

Acknowledgment

• This request meets Powertools for AWS Lambda (TypeScript) Tenets
• Should this be considered in other Powertools for AWS Lambda languages? i.e. Python, Java, and .NET

Future readers

Please react with 👍 and your use case to help us understand customer demand.

[dreamorosi]

Thanks for opening this issue - what would be the advantage of doing this?

Overall I'm not a big fan of importing things that are not modules as such, especially because it introduces a bunch of JSON files into the TS engine for a one-time usage.

Generally speaking we want this operation to be sync and blocking, so I'm also not sure about the advantage of making it async.

[svozza]

My preference is to use JavaScript standards such as import rather than relying on implementation details of Node.js like __dirname. Importing JSON is a perfectly valid use case, as seen by the TC39 JSON modules proposal that is now at stage 3 and has been implemented by all major JS engines.

Regarding synchronous methods, again, my preference is to avoid sync methods in JS wherever possible. We are currently doing 100+ synchronous reads of these events and this number is only going to increase as we increase event test coverage and schema types (#3301 alone adds 11 new JSON files) so we're blocking the event loop on every single schema test.

However, I don't feel particularly strongly about this so happy to close if you don't see the benefit: that's why I opened the issue rather than jumping straight to a PR.

[am29d]

I remember I had a similar motivation when I started working on parser and had to deal with the example event imports. One of the issues back then was to tweak tsconfig for json imports, can't remember the specifics.

While Importing 100+ files will speed up tests because it's not blocking and caching, the files will remain in the cache which might have impact on IDE. Imho it's hard to roll the dice without clear numbers, and I think it'd be too much effort to we'd need to understand the impact. We could switch but would still not know for sure what the implications really are without measuring.

I am for keeping it atm like it is. If anyone want's to dive a bit deeper into this area and provide numbers, feel free.

[svozza]

I didn't have any problems with tsconfig.json, I just added two fields to packages/parser/tests/tsconfig.json:

{
  "extends": "../tsconfig.json",
  "compilerOptions": {
    "rootDir": "../",
    "noEmit": true,
    "resolveJsonModule": true
  },
  "include": ["../src/**/*", "./**/*", "./events/*.json"] // you must explicitly add the .json extension
}

However, I agree, makes sense to measure the effect. If I have time I will give it a go.

[svozza]

There doesn't seem to be a significant difference between the two so it doesn't seem worth changing. Top one is sync, bottom one is async:

[dreamorosi]

Thank you for dedicating time to benchmark the two methods, kudos for the data driven approach.

I agree with your assessment, however I'd like to also share we're aware that this area of the tests can be improved and streamlined.

Specifically we want to reduce the I/O operation altogether by instead reading each event file category (i.e. SQS) only once in the SQS tests and then clone these events and/or extend them as needed throughout the different cases.

We've made some initial efforts and we were able to cut down on a lot of files by simply changing one or two fields in certain test cases.

I started working on a PR which quickly became enormous and that I eventually closed. We're now adopting a more gradual approach and changing the way we load files as we touch tests, similar to what is being done in #3328.

[svozza]

Specifically we want to reduce the I/O operation altogether by instead reading each event file category (i.e. SQS) only once in the SQS tests and then clone these events and/or extend them as needed throughout the different cases.

We've made some initial efforts and we were able to cut down on a lot of files by simply changing one or two fields in certain test cases.

Yeah, I had actually considered going down this route for the AppSync tests but just followed the pattern that I saw in the codebase at the time. I think it's a good idea and I'm happy to change the tests in #3301 to use this approach.

[dreamorosi]

Happy to have these changes in that PR @svozza, yes.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.