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

feat: export metadata object in stage 2 #122

Merged
merged 3 commits into from
Sep 14, 2022
Merged

feat: export metadata object in stage 2 #122

merged 3 commits into from
Sep 14, 2022

Conversation

eduardoboucas
Copy link
Member

Functionally, this PR adds a metadata object to the exports of the stage 2 file. Right now, this object contains just a url property containing the file path of each function, like so:

// Existing export
export const functions = {
  func1: [Function],
  func2: [Function]
}

// New export
export const metadata = {
  func1: {
    url: "file:///some/path/to/func1.ts",
  },
  func2: {
    url: "file:///some/path/to/func2.ts",
  }
}

This will allow the bootstrap code to match log locations to function names.

Also, it includes some housekeeping:

  • Node tests were moved from test/ to test/node/
  • Deno tests added to test/deno/
  • CI updated to run Deno tests
  • Added stage 2 test

@eduardoboucas eduardoboucas added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 14, 2022
@eduardoboucas eduardoboucas requested a review from a team September 14, 2022 17:01
import { getStage2Entry } from '../../deno/lib/stage2.ts'
import { virtualRoot } from '../../deno/lib/consts.ts'

Deno.test('`getStage2Entry` returns a valid stage 2 file', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was long overdue because we're dynamically generating code so it's good that we validate it as early as possible.

Copy link
Member

@mraerino mraerino left a comment

Choose a reason for hiding this comment

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

always confused that tests are not colocated with the implementation like in Go 😅

Comment on lines 28 to 34
const metadataLines = lines.map(({ metadata, name }) => {
const metadataString = JSON.stringify(metadata)

return [importLines, exportDeclaration].join('\n\n')
return `"${name}":${metadataString}`
})
const functionsExport = `export const functions = {${exportLines}};`
const metadataExport = `export const metadata = {${metadataLines}};`
Copy link
Member

Choose a reason for hiding this comment

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

this scares me a bit. why are you doing this line by line instead of just calling JSON.stringify on a whole object?
also: did you forget a .join(",")?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! Updated in 71bed63.


await Deno.writeTextFile(stage2Path, normalizedStage2)

const mod = await import(stage2Path)
Copy link
Member

Choose a reason for hiding this comment

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

wow, importing code in the code that generated it is next level 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, right?! 🤯

}),
{},
)
const functionsExport = `export const functions = {${exportLines}};`
Copy link
Member

Choose a reason for hiding this comment

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

could you do the same here if you're already on it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not serialisable to JSON, though. The output of this will be something like:

export const functions = {"func1": func0, "func2": func1};

... where func0 and func1 are the imported modules.

@khendrikse
Copy link
Contributor

khendrikse commented Sep 14, 2022

I saw the change of moving tests into more organised folders. I do really like structuring all of this a little bit. I remember looking into the code there are files in the src folder that are organised in folders, and some that are not. Maybe it might be interesting in the future to organise the src folder in general. Because it might give a little bit more context to both the code and the tests. (for example, we have bundle.ts that is just an export of an interface, while there are other files that export functions).

Right now I'd almost say it might be clearer for the deno tests to just be a part of the deno folder because it'll clearly show the existing tests, and might lead to an incentive to write more when changing something there or adding something there.

Also: total nitpicking here, no prioritisation at all :)

@eduardoboucas
Copy link
Member Author

I saw the change of moving tests into more organised folders. I do really like structuring all of this a little bit. I remember looking into the code there are files in the src folder that are organised in folders, and some that are not. Maybe it might be interesting in the future to organise the src folder in general. Because it might give a little bit more context to both the code and the tests. (for example, we have bundle.ts that is just an export of an interface, while there are other files that export functions).

Right now I'd almost say it might be clearer for the deno tests to just be a part of the deno folder because it'll clearly show the existing tests, and might lead to an incentive to write more when changing something there or adding something there.

Also: total nitpicking here, no prioritisation at all :)

Good thoughts! Let's iterate on this.

@kodiakhq kodiakhq bot merged commit 99214c7 into main Sep 14, 2022
Skn0tt pushed a commit to netlify/build that referenced this pull request Apr 23, 2024
* feat: export `metadata` object in stage 2

* refactor: simplify metadata generation

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants