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

fix: include route prefix in vercel func names, fix #8401 #8408

Merged
merged 7 commits into from
Sep 6, 2023

Conversation

slawekkolodziej
Copy link
Contributor

Changes

Recently updated Vercel adapter has a bug. If two routes have identical file names but different directory structures, the adapter will produce only one serverless function and assign it to both routes.

This PR fixes the problem.

Some more details: #8401

Testing

Within vercel adapter I created 'serverless-with-dynamic-routes' fixture and added necessary tests. If you run those tests against current main, the will fail.

Docs

No need to update docs. This PR only fixes a rather major bug and brings the expected behavior.

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 4, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 4, 2023

🦋 Changeset detected

Latest commit: fd2c50c

The changes in this PR will be included in the next version bump.

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

@slawekkolodziej
Copy link
Contributor Author

Since my proposed change injects route path to file name, I begun to wonder what is the maximum allowed length for serverless function name in Vercel build output? Since I couldn't find the answer in their documentation, I had to run some manual tests.

The answer is: less than 240 characters. If *.func folder has 240 characters or more, Vercel responds with

Error: ENAMETOOLONG: name too long

With that in mind, I think that the proposed solution is fine for the time being, just to fix the original issue. However ideally there should be a mechanism that limits function name length.

@matoous
Copy link

matoous commented Sep 5, 2023

thought: should we at least throw an error if the length is exceeded? Other option would be to hash the path into fixed-size string and use only the filename, e.g. src/api/some-endpoint-path-here/sub-folder-there/another-subfolder-here/and-we-keep-nesting-forever-and-ever/until-we-get-to/the-final-file.ts would be f526d3493c217e4aaf128fa1cb0f553de92cf30fce348a2b00231c8638fa9626.the-final-file.ts (although this make it way less readable and harder to debug) (the hash length could be reduced).

@slawekkolodziej
Copy link
Contributor Author

I thought about limiting the file name to a sane number of characters and adding short content hash to the end.

I like to have route included in the function name - as you said, it simplifies debugging and makes debug.

Now when I think about it, we could ditch entry- and -astro parts of function names completely, to improve monitoring even more:

Consider what my original change generates:
what-code-generates-now

After some adjustments, Vercel can accept this too:
what-we-can-have

@ematipico
Copy link
Member

Now when I think about it, we could ditch entry- and -astro parts of function names completely

  • entry comes from the core; maybe we should remove it from there?
  • .astro is just the extension of the file, you would need to remove .js, .ts, etc., right?

@slawekkolodziej
Copy link
Contributor Author

  • .astro is just the extension of the file, you would need to remove .js, .ts, etc., right?

The complete file extension is .astro.mjs, original code already removed .mjs.

@slawekkolodziej
Copy link
Contributor Author

I simplified the code a bit, now my routes looks as clean as they can get 💚

Screenshot 2023-09-05 at 14 54 57

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I left some comments. I think overall, we could release this fix.

packages/integrations/vercel/src/serverless/adapter.ts Outdated Show resolved Hide resolved
packages/integrations/vercel/src/serverless/adapter.ts Outdated Show resolved Hide resolved
@ematipico ematipico requested review from matthewp and bluwy September 5, 2023 13:01
@ematipico ematipico merged commit 9ffa1a8 into withastro:main Sep 6, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants