feat(netlify): support included/excluded files #325

ascorbic · 2024-07-11T14:59:10Z

Changes

Adds "includedFiles" and "excludedFiles" options. These allow extra files to be deployed in the SSR function bundle. It globs these files at build time, and includes them in the file list that is deployed in the function. This PR also ensures that process.cwd matches the site root during SSR.

Testing

Adds new test fixture and tests.

Docs

This needs docs. The changeset includes a lot of the required info, as do the types.

changeset-bot · 2024-07-11T14:59:14Z

🦋 Changeset detected

Latest commit: 1a2319f

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

This PR includes changesets to release 2 packages

Name	Type
@astrojs/netlify	Major
@test/netlify-hosted-astro-project	Patch

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

.changeset/itchy-donuts-love.md

packages/netlify/src/index.ts

florian-lefebvre · 2024-07-11T15:50:37Z

packages/netlify/src/ssr-function.ts

 		return async function handler(request: Request, context: Context) {
+			// This entrypoint will be deep inside the directory structure below cwd
+			// We chdir to the root so that we can resolve the correct paths at runtime
+			process.chdir(root);


Are we sure it can't break any existing app?

As far as I can work out, existing apps that rely on cwd would either have unchanged behaviour (if they're not a monorepo), or would already be broken (if they are using a monorepo)

I don't understand what the "correct paths at runtime" refers to here. It feels dangerous to call chdir() as a request handler. Can you explain where is resolving from the cwd?

If a site uses filesystem functions at runtime, this is the only way it can correctly load the files

I don't think using filesystem functions is something Astro supports in the runtime? Files could be moved during bundling and it wouldn't work later.

Is process.chdir() needed for this PR?

We don't prevent people using fs functions, so I'd imagine lots of people do. This PR was prompted by an issue where a library was trying to load i18n data and it wasn't found. This part fixes precisely that issue: of files being moved during bundling. Without that there's not a lot of point to allowing users to include files in the bundle, because the user code won't be able to find them.

I don't feel like fs functions are a common thing especially when dealing in a bundled runtime. There's Vite APIs like import.meta.glob, ?url, and ?raw for them. Fs functions are also likely to fail elsewhere than only on Netlify, and so we shouldn't support and encourage them.

I figured the benefit with includeFiles and excludeFiles are like Vercel's option, because nft has trouble sometimes detecting every file. Especially inside node_modules where libraries use fs relatively, those code are not processed by Vite so we could use the option for them, however if they resolve relatively I'm not sure how cwd helps here. And if the library do resolve from cwd, it would've been generally a problem if an Astro is also hosted on a subdirectory or any other fine setup.

I'm not really comfortable with keeping this in the adapter. It feels surprising to me that it could change the cwd internally and on every request.

They are like Vercel's option. In fact they use almost the same implementation. With libraries that use fs functions, they usually accept a directory as an option or resolve relative to cwd. It's very common for i18n libraries such as i18next, for example.

If we're about to do a major I can do a breaking changeset for the chdir part.

sarah11918

This looks super helpful, @ascorbic and the docs are fabulous!

Just a couple of quick thoughts mostly on formatting.

(In particular, we do tend to use inline code when naming a feature, not quotation marks. And both inline AND quotation marks are reserved for when that's how the feature would be shown in the reader's code (e.g. output: 'server'. (Though, we're more relaxed with hybrid and server just written on their own.)

Otherwise, great intro, great explanation, and helpful code example. What more could I possibly want? 😄

.changeset/itchy-donuts-love.md

Co-authored-by: Sarah Rainsberger <[email protected]>

ascorbic · 2024-07-12T11:11:36Z

!preview include-files

github-actions · 2024-07-12T11:12:20Z

Snapshots have been released for the following packages:

@astrojs/netlify@experimental--include-files

Publish Log

🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--include-files tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info @astrojs/cloudflare
🦋  info npm info @astrojs/netlify
🦋  warn @astrojs/cloudflare is not being published because version 11.0.1 is already published on npm
🦋  info @astrojs/netlify is being published because our local version (0.0.0-include-files-20240712111209) has not been published on npm
🦋  info Publishing "@astrojs/netlify" at "0.0.0-include-files-20240712111209"
🦋  success packages published successfully:
🦋  @astrojs/[email protected]
🦋  Creating git tag...
🦋  New tag:  @astrojs/[email protected]

Build Log

 WARN  Unsupported engine: wanted: {"node":">=20.15.0"} (current: {"node":"v18.20.3","pnpm":"9.4.0"})

> [email protected] build /home/runner/work/adapters/adapters
> turbo run build --filter="@astrojs/*"

• Packages in scope: @astrojs/cloudflare, @astrojs/netlify, @astrojs/test-utils
• Running build in 3 packages
• Remote caching disabled
::group::@astrojs/netlify:build
cache miss, executing 263e5d99e0f2b801

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/netlify
> tsc

::endgroup::
::group::@astrojs/cloudflare:build
cache miss, executing 81371b8c47fd6c9f

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/cloudflare
> tsc

::endgroup::

 Tasks:    2 successful, 2 total
Cached:    0 cached, 2 total
  Time:    3.605s

alexanderniebuhr · 2024-07-23T08:33:22Z

@ascorbic Do you want to land this before the next minor of the Netlify Adapter?

ascorbic · 2024-07-23T09:47:18Z

@alexanderniebuhr Ideally, yes. It's still waiting on an approval

alexanderniebuhr · 2024-07-23T09:48:37Z

I already looked at it and quite liked it, but I can review it later today, so we can land it before we cut the release :)

.changeset/itchy-donuts-love.md

Co-authored-by: Bjorn Lu <[email protected]>

packages/netlify/src/index.ts

packages/netlify/test/functions/fixtures/includes/astro.config.mjs

packages/netlify/src/ssr-function.ts

alexanderniebuhr

I miss specific platform knowledge but I like the feature and implementation looks good and has tests.

sarah11918

Just left a suggestion of a more compact style, if you'd like one!

.changeset/itchy-donuts-love.md

Co-authored-by: Sarah Rainsberger <[email protected]>

alexanderniebuhr · 2024-08-03T06:11:14Z

I was not able to wait for this PR to make it into the release, I needed to ship Cloudflare bugfixes.. Are all discussions resolved?

sarah11918 · 2024-08-05T13:02:27Z

The changeset is fine by docs, but we will need documentation in the Netlify adapter page in docs. A new <h3> under Usage seems like a good place, I think? https://docs.astro.build/en/guides/integrations-guide/netlify/#usage

As Matt wrote, it can take very much the form of what's in the changeset, plus any other important characteristics to document (because I said the changeset doesn't have to be exhaustive -- but there may be other details appropriate for full documentation).

ascorbic · 2024-09-05T13:02:41Z

!preview include-files

github-actions · 2024-09-05T13:03:29Z

Snapshots have been released for the following packages:

@astrojs/netlify@experimental--include-files

Publish Log

🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--include-files tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info @astrojs/cloudflare
🦋  info npm info @astrojs/netlify
🦋  info npm info @astrojs/node
🦋  info npm info @astrojs/vercel
🦋  warn @astrojs/cloudflare is not being published because version 11.0.4 is already published on npm
🦋  info @astrojs/netlify is being published because our local version (0.0.0-include-files-20240905130315) has not been published on npm
🦋  warn @astrojs/node is not being published because version 8.3.3 is already published on npm
🦋  warn @astrojs/vercel is not being published because version 7.8.0 is already published on npm
🦋  info Publishing "@astrojs/netlify" at "0.0.0-include-files-20240905130315"
🦋  success packages published successfully:
🦋  @astrojs/[email protected]
🦋  Creating git tag...
🦋  New tag:  @astrojs/[email protected]

Build Log


> [email protected] build /home/runner/work/adapters/adapters
> turbo run build --filter="@astrojs/*"

• Packages in scope: @astrojs/cloudflare, @astrojs/netlify, @astrojs/node, @astrojs/test-utils, @astrojs/vercel
• Running build in 5 packages
• Remote caching disabled
::group::@astrojs/node:build
cache miss, executing 500254f597799cf1

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/node
> tsc

::endgroup::
::group::@astrojs/netlify:build
cache miss, executing 2759776ada86b638

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/netlify
> tsc

::endgroup::
::group::@astrojs/vercel:build
cache miss, executing 6e63e7a22cb615dd

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/vercel
> tsc

::endgroup::
::group::@astrojs/cloudflare:build
cache miss, executing b168b1ba31cb4844

> @astrojs/[email protected] build /home/runner/work/adapters/adapters/packages/cloudflare
> tsc

::endgroup::

 Tasks:    4 successful, 4 total
Cached:    0 cached, 4 total
  Time:    6.222s

github-actions bot added the pkg: netlify label Jul 11, 2024

ascorbic force-pushed the include-files branch from 1c4a863 to a2037a4 Compare July 11, 2024 15:02

feat(netlify): support included/excluded files

e7206e2

ascorbic force-pushed the include-files branch from a2037a4 to e7206e2 Compare July 11, 2024 15:10

florian-lefebvre reviewed Jul 11, 2024

View reviewed changes

sarah11918 reviewed Jul 11, 2024

View reviewed changes

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

ascorbic and others added 3 commits July 11, 2024 18:55

Update .changeset/itchy-donuts-love.md

cedb642

Co-authored-by: Sarah Rainsberger <[email protected]>

Change from review

bc08199

Merge branch 'main' into include-files

171b832

Merge branch 'main' into include-files

4db5b4d

alexanderniebuhr mentioned this pull request Jul 23, 2024

[ci] release #329

Merged

alexanderniebuhr self-requested a review July 23, 2024 09:48

bluwy reviewed Jul 23, 2024

View reviewed changes

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

Update .changeset/itchy-donuts-love.md

4cdf7ae

Co-authored-by: Bjorn Lu <[email protected]>

ematipico reviewed Jul 23, 2024

View reviewed changes

packages/netlify/src/index.ts Show resolved Hide resolved

packages/netlify/test/functions/fixtures/includes/astro.config.mjs Show resolved Hide resolved

Add test

4595d84

ascorbic force-pushed the include-files branch from 50a3852 to 4595d84 Compare July 24, 2024 09:06

Format

2c98705

ascorbic requested a review from ematipico July 24, 2024 09:15

ematipico reviewed Jul 24, 2024

View reviewed changes

packages/netlify/src/ssr-function.ts Outdated Show resolved Hide resolved

Add try/catch to chdir

76fab50

alexanderniebuhr approved these changes Jul 24, 2024

View reviewed changes

sarah11918 reviewed Jul 26, 2024

View reviewed changes

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

.changeset/itchy-donuts-love.md Outdated Show resolved Hide resolved

Apply suggestions from code review

d68f5b2

Co-authored-by: Sarah Rainsberger <[email protected]>

Merge branch 'main' into include-files

5570bdf

ascorbic force-pushed the include-files branch from 74fcbdb to 5570bdf Compare September 4, 2024 10:50

ascorbic added 2 commits September 5, 2024 11:02

Use more robust root detection

1d198eb

Changeset

1a2319f

withastro deleted a comment from github-actions bot Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(netlify): support included/excluded files #325

feat(netlify): support included/excluded files #325

ascorbic commented Jul 11, 2024

changeset-bot bot commented Jul 11, 2024 •

edited

Loading

florian-lefebvre Jul 11, 2024

ascorbic Jul 11, 2024

bluwy Jul 23, 2024

ascorbic Jul 24, 2024

bluwy Jul 24, 2024 •

edited

Loading

ascorbic Jul 24, 2024

bluwy Jul 24, 2024 •

edited

Loading

ascorbic Jul 25, 2024

ascorbic Sep 4, 2024

sarah11918 left a comment

ascorbic commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

alexanderniebuhr commented Jul 23, 2024

ascorbic commented Jul 23, 2024

alexanderniebuhr commented Jul 23, 2024

alexanderniebuhr left a comment •

edited

Loading

sarah11918 left a comment

alexanderniebuhr commented Aug 3, 2024

sarah11918 commented Aug 5, 2024 •

edited

Loading

ascorbic commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

feat(netlify): support included/excluded files #325

Are you sure you want to change the base?

feat(netlify): support included/excluded files #325

Conversation

ascorbic commented Jul 11, 2024

Changes

Testing

Docs

changeset-bot bot commented Jul 11, 2024 • edited Loading

🦋 Changeset detected

florian-lefebvre Jul 11, 2024

Choose a reason for hiding this comment

ascorbic Jul 11, 2024

Choose a reason for hiding this comment

bluwy Jul 23, 2024

Choose a reason for hiding this comment

ascorbic Jul 24, 2024

Choose a reason for hiding this comment

bluwy Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

ascorbic Jul 24, 2024

Choose a reason for hiding this comment

bluwy Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

ascorbic Jul 25, 2024

Choose a reason for hiding this comment

ascorbic Sep 4, 2024

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

ascorbic commented Jul 12, 2024

github-actions bot commented Jul 12, 2024

alexanderniebuhr commented Jul 23, 2024

ascorbic commented Jul 23, 2024

alexanderniebuhr commented Jul 23, 2024

alexanderniebuhr left a comment • edited Loading

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

alexanderniebuhr commented Aug 3, 2024

sarah11918 commented Aug 5, 2024 • edited Loading

ascorbic commented Sep 5, 2024

github-actions bot commented Sep 5, 2024

changeset-bot bot commented Jul 11, 2024 •

edited

Loading

bluwy Jul 24, 2024 •

edited

Loading

bluwy Jul 24, 2024 •

edited

Loading

alexanderniebuhr left a comment •

edited

Loading

sarah11918 commented Aug 5, 2024 •

edited

Loading