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: add Frameworks API #5668

Merged
merged 18 commits into from
Jun 21, 2024
Merged

feat: add Frameworks API #5668

merged 18 commits into from
Jun 21, 2024

Conversation

eduardoboucas
Copy link
Member

@eduardoboucas eduardoboucas commented May 26, 2024

Summary

Extends the Deploy Configuration API and renames it to Frameworks API. The PR ended up getting larger than I'd want, but I felt it was important to land the different endpoints together. Plus, the bulk of the diff is changes to tests.

To make it easier to review, I've separated the different parts of the API into different commits. I recommend reviewing them one by one, in the order below. I'll also add inline comments whenever I feel it helps.

  1. e411f4f: Extend the properties that can be changed from the config.json endpoint and rename the feature to Frameworks API
  2. f25f5dd and d768495: Add new endpoint for uploading blobs
  3. 6947d46 and c7ce32f: Add new endpoint for functions
  4. 5e7293a: Add new endpoint for edge functions
  5. ed08b1c: Remove the feature flag introduced in feat: initial support for Deploy Configuration API #5509, which has now been fully rolled out

Copy link
Contributor

This pull request adds or modifies JavaScript (.js, .cjs, .mjs) files.
Consider converting them to TypeScript.

logDryRunStep({ logs, step, index, netlifyConfig, eventWidth, stepsCount })
})
successSteps
.filter((step) => !step.quiet)
Copy link
Member Author

Choose a reason for hiding this comment

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

Filtering before iterating means we have the right value for index.

storeOpts.experimentalRegion = 'auto'
}

const blobStore = getDeployStore(storeOpts)
const keys = await getKeysToUpload(blobs.directory)
const blobsToUpload = blobs.apiVersion >= 3 ? await getBlobs(blobs.directory) : await getKeysToUpload(blobs.directory)
Copy link
Member Author

Choose a reason for hiding this comment

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

blobsToUpload is now an array of objects with the following properties:

  • key: The blob key
  • contentPath: The absolute path to the blob content file
  • metadataPath: The absolute path to the blob metadata file (doesn't mean that one exists)

coreStepDescription: () => '',
condition: ({ featureFlags }) => featureFlags?.netlify_build_deploy_configuration_api,
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the feature flag because it's been fully rolled out.

generatedFunctionsPath = frameworksAPISrcPath
}

const frameworkImportMap = resolve(
Copy link
Member Author

Choose a reason for hiding this comment

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

We now look for import maps on a specific location, as opposed to reading that from the legacy manifest.json file.

https://www.notion.so/netlify/Frameworks-API-documentation-b35d9fd4efcd4979a520b0607907001e?pvs=4#499d17a1cfbb4c8da316c2216b51bf77

* result is an array with the blob key, the path to its data file, and the
* path to its metadata file.
*/
export const getKeysToUpload = async (blobsDir: string) => {
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 the old method for the legacy APIs (file-based uploads and Deploy Configuration API). We're just changing the shape of the return value to be an array of objects.

})
})

return blobsToUpload
}

/** Read a file and its metadata file from the blobs directory */
export const getFileWithMetadata = 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.

Because of the changes we made to getKeysToUpload, this method is now simpler and receives the path of the content and metadata files, as opposed to having to compute them itself.

@@ -37,10 +37,6 @@ const NORMALIZE_REGEXPS = [
/(^|[ "'(=])((?:\.{0,2}|([A-Z]:)|file:\/\/)(\/[^ "')\n]+))/gm,

(_, prefix, pathMatch, winDrive, pathTrail) => {
if (pathMatch.includes('/$netlify-snapshot-preserve/')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We're not using this.

🔪 snapshots

@eduardoboucas eduardoboucas marked this pull request as ready for review June 13, 2024 11:22
@eduardoboucas eduardoboucas requested review from a team as code owners June 13, 2024 11:22
Copy link
Contributor

@orinokai orinokai left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

})
})

return blobsToUpload
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be interesting to understand the rationale for using forEach here instead of map

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! No reason, this should definitely be a map. I'll do this in a follow-up.

@eduardoboucas eduardoboucas merged commit ec3bcc8 into main Jun 21, 2024
37 checks passed
@eduardoboucas eduardoboucas deleted the feat/frameworks-api branch June 21, 2024 09:57
This was referenced Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants