Skip to content
This repository was archived by the owner on Apr 24, 2023. It is now read-only.

Enforce Function File Contract earlier#27

Merged
WilliamBergamin merged 13 commits intoslackapi:mainfrom
WilliamBergamin:early-function-contract
Feb 2, 2023
Merged

Enforce Function File Contract earlier#27
WilliamBergamin merged 13 commits intoslackapi:mainfrom
WilliamBergamin:early-function-contract

Conversation

@WilliamBergamin
Copy link
Copy Markdown
Contributor

@WilliamBergamin WilliamBergamin commented Jan 31, 2023

Summary

The goal of this PR is to allow the deno slack builder to detect if a function file has a default export and notify the user is it does not.

ticket: HERMES-2867

Test:

  1. Create a Deno project using the CLI or cd to an existing project
  2. Run
    deno run -r --allow-read --allow-write --allow-net --allow-run --config=deno.jsonc 
    https://raw.githubusercontent.com/WilliamBergamin/deno-slack-builder/early-function-contract/src/mod.ts
    everything should pass
  3. Edit a file containing a Function and remove the export default handler
  4. Run
    deno run -r --allow-read --allow-write --allow-net --allow-run --config=deno.jsonc 
    https://raw.githubusercontent.com/WilliamBergamin/deno-slack-builder/early-function-contract/src/mod.ts
    This time something should fail and the output should contain this
    error: Uncaught (in promise) Error: File: {YOUR PATH TO BROKEN FILE}, containing your function does not define a default export handler.
    

I'm looking for feedback on additional testing, making sure this will work with the other components of the system

Requirements

@WilliamBergamin WilliamBergamin added bug Something isn't working semver:minor requires a minor version number bump labels Jan 31, 2023
@WilliamBergamin WilliamBergamin requested a review from a team as a code owner January 31, 2023 23:29
@WilliamBergamin WilliamBergamin self-assigned this Jan 31, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 31, 2023

Codecov Report

Merging #27 (99d8fe7) into main (daf2b41) will increase coverage by 20.52%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #27       +/-   ##
===========================================
+ Coverage   10.60%   31.13%   +20.52%     
===========================================
  Files           4        4               
  Lines         198      212       +14     
  Branches        0       12       +12     
===========================================
+ Hits           21       66       +45     
+ Misses        177      145       -32     
- Partials        0        1        +1     
Impacted Files Coverage Δ
src/dev_deps.ts 100.00% <100.00%> (ø)
src/functions.ts 39.82% <100.00%> (+38.82%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@WilliamBergamin
Copy link
Copy Markdown
Contributor Author

Need to do more testing before reopening this PR

Copy link
Copy Markdown
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few small nitpicks, looks good otherwise. Any larger tasks coming out of any of my comments can be filed as separate issues and addressed later.

deno.jsonc Outdated
"tasks": {
"test": "deno fmt --check && deno lint && deno test src",
"coverage": "deno test --coverage=.coverage src && deno coverage --exclude=fixtures --exclude=test .coverage"
"test": "deno fmt --check && deno lint && deno test src --allow-read",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small suggestion, the flags should go before the directory parameter, just to align with deno test CLI help/usage docs.

Suggested change
"test": "deno fmt --check && deno lint && deno test src --allow-read",
"test": "deno fmt --check && deno lint && deno test --allow-read src",

const functionPathHasDefaultExport = async (
functionFilePath: string,
) => {
const functionModule = await import(`file://${functionFilePath}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Random thought: we could potentially enforce the type of default export returned by userland function code, ensuring that they are e.g. exporting the return of SlackFunction.

Not a blocker for this PR but just food for thought / future enhancement.

Copy link
Copy Markdown
Contributor

@filmaj filmaj Feb 1, 2023

Choose a reason for hiding this comment

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

Thinking more about how these default exports are consumed at runtime, and in particular:

Perhaps we should ensure that the default export is, at least, a function? Maybe not as far as my previous comment suggested (to check the specific return type), but maybe as simple as a typeof functionModule.default == 'function' check?

Adding this additional check would prevent a developer from doing something silly like export default 3; or some other non-function type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good call to ensure the default export is a function, like that the footprint is minimal

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ive tested and implemented the typeof functionModule.default == 'function' check good idea @filmaj 💯

assertExists(validateAndCreateFunctions);
});

Deno.test("Function files should have a default export", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am a bit of a pedant when it comes to test names - I really like for the test name to make clear what the assumptions around a test are, as well as what the expected behaviour is. I'd change this to something like:

Suggested change
Deno.test("Function files should have a default export", async () => {
Deno.test("validateAndCreateFunctions should not throw an exception when fed a function file that has a default export", async () => {

@WilliamBergamin WilliamBergamin merged commit 1f79e7a into slackapi:main Feb 2, 2023
@WilliamBergamin WilliamBergamin deleted the early-function-contract branch February 2, 2023 23:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working semver:minor requires a minor version number bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants