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(netlify): Netlify Adapter v4 #84

Merged
merged 64 commits into from
Dec 17, 2023
Merged

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Dec 4, 2023

Changes

This PR overhauls the Netlify adapter, aiming to make best use of Netlifys platform & streamlining config.
Read the changeset for a detailed overview, but here's the main changes:

  • Middleware runs on all pages, not just SSR routes, independent of config
  • Netlify context automatically available via locals, no need for netlify-edge-middleware.ts file
  • Support for Netlify Image CDN
  • Removal of On-Demand Builders, because we recommend frameworks to use standard caching headers instead
  • Removal of unneeded config options

Testing

Tests were brought up-to-date.

Docs

README was updated.

@lilnasy
Copy link
Contributor

lilnasy commented Dec 11, 2023

Do you think it's a good idea to offer a mocked netlify context in dev mode?

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 11, 2023

Do you think it's a good idea to offer a mocked netlify context in dev mode?

That would be nice, I guess! We already mock the context in netlify dev, would be good for that to extend into Astro dev. How could that be implemented?

@lilnasy
Copy link
Contributor

lilnasy commented Dec 11, 2023

You'll have to add a vite dev server (connect server) middleware, and set request[Symbol.for("astro.locals")]. Here's what it looked like in my PR: 53775ba.

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 11, 2023

Wow, that's pretty cool! I'll definitely implement that.

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 11, 2023

Besides that, are there any other changes you'd like me to make @lilnasy @alexanderniebuhr @ematipico? From what i'm seeing, all the big ones are addressed and the only missing thing is the docs review. What else do we need to work towards merging?

@lilnasy
Copy link
Contributor

lilnasy commented Dec 11, 2023

I will be testing out the functionality later today.

As for the big picture, I would be more conformable if the default story was "node only" - for both SSR and middleware, with edge the opt-in.

@alexanderniebuhr
Copy link
Member

Just checking in, I don't have anything to add for now. But I'll make sure to get the docs review this week for you! :)

Skn0tt and others added 2 commits December 12, 2023 13:17
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 12, 2023

I've implemented the mocked context in 4d4c245 based off your commit @lilnays. Tried giving you credit for that using Co-Authored-By, but somehow GitHub didn't like it and isn't showing your face on the commit :/

As for the big picture, I would be more conformable if the default story was "node only" - for both SSR and middleware, with edge the opt-in.

Cool, will do! I thought the edge was already opt-in, and docs + comments said that - but I noticed the actual code was making it opt-out. Fixed that in 0ac9466.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Whew! A major release is exciting! 🥳

Left some suggestions for docs here! (Note: since there was a comment, yes, informing readers of CHANGES goes in the changeset... and there were a lot, so this is the place for them! The README is for "how things just are" and is NOT the place to discuss changes from past versions. So the content there should just be about how it works now.)

packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
packages/netlify/README.md Outdated Show resolved Hide resolved
.changeset/ten-cougars-pretend.md Outdated Show resolved Hide resolved
.changeset/ten-cougars-pretend.md Outdated Show resolved Hide resolved
.changeset/ten-cougars-pretend.md Outdated Show resolved Hide resolved
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 13, 2023

@sarah11918 wow, what a review! I always love docs review because the prose reads so much better after that :D I've made most of the changes you requested, and left comments to the two remaining ones.

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.

Thank you for addressing all my comments!

Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

Unblocking, since we have docs review

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 15, 2023

Awesome, thanks for approving! I think now we're only waiting for the docs copy to be finalised.

Skn0tt and others added 2 commits December 15, 2023 16:02
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@alexanderniebuhr
Copy link
Member

@Skn0tt Just checking in, if you are ready, I'll do another quick sanity check later tonight and get this merged 🚀

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 15, 2023

Sounds great! Maybe we can merge today + release early monday, so we can respond better to questions in Discord.

@alexanderniebuhr
Copy link
Member

@Skn0tt No problem, I'll coordinate everything, so we can get the release on Monday morning CET timezone! :)

@alexanderniebuhr alexanderniebuhr self-assigned this Dec 15, 2023
Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

I just did another run through the changes, and they lgtm.
Thank you for the PR! @Skn0tt

I'll get this merged now and released on Monday morning! 🚀

@alexanderniebuhr alexanderniebuhr changed the title feat!(netlify): Overhaul Netlify Adapter feat(netlify): Netlify Adapter v4 Dec 17, 2023
@alexanderniebuhr alexanderniebuhr merged commit ca64544 into withastro:main Dec 17, 2023
8 checks passed
@github-actions github-actions bot mentioned this pull request Dec 17, 2023
@Skn0tt
Copy link
Contributor Author

Skn0tt commented Dec 18, 2023

Awesome, thanks for all the reviews! I'll keep an eye on Discord and GitHub for any issues that pop up. If I miss any, feel free to tag me in related issues - happy to help.

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

6 participants