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

Revert server action optimization #69925

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Revert server action optimization #69925

merged 2 commits into from
Sep 10, 2024

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Sep 10, 2024

Reverting unused server actions optimization on 14.2 to avoid complex nested server actions failing. We're addressing the documentation updates in vercel/ai#2956

Revert #69788
Revert #69178

@ijjk
Copy link
Member

ijjk commented Sep 10, 2024

Stats from current PR

Default Build
General
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
buildDuration 16.5s 14.9s N/A
buildDurationCached 8s 6.9s N/A
nodeModulesSize 200 MB 200 MB N/A
nextStartRea..uration (ms) 405ms 404ms N/A
Client Bundles (main, webpack)
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
1103-HASH.js gzip 31.6 kB 31.6 kB N/A
1a9f679d-HASH.js gzip 53.7 kB 53.7 kB N/A
335-HASH.js gzip 5.05 kB 5.05 kB
7953.HASH.js gzip 181 B 181 B
framework-HASH.js gzip 44.9 kB 44.9 kB
main-app-HASH.js gzip 243 B 244 B N/A
main-HASH.js gzip 32.2 kB 32.2 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 50.1 kB 50.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
_app-HASH.js gzip 195 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 501 B 506 B N/A
css-HASH.js gzip 323 B 324 B N/A
dynamic-HASH.js gzip 1.82 kB 1.82 kB N/A
edge-ssr-HASH.js gzip 259 B 258 B N/A
head-HASH.js gzip 350 B 352 B N/A
hooks-HASH.js gzip 372 B 371 B N/A
image-HASH.js gzip 4.23 kB 4.23 kB
index-HASH.js gzip 259 B 258 B N/A
link-HASH.js gzip 2.68 kB 2.68 kB N/A
routerDirect..HASH.js gzip 316 B 315 B N/A
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 311 B 310 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 4.9 kB 4.9 kB
Client Build Manifests
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
_buildManifest.js gzip 483 B 483 B
Overall change 483 B 483 B
Rendered Page Sizes
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
index.html gzip 527 B 529 B N/A
link.html gzip 541 B 541 B
withRouter.html gzip 523 B 525 B N/A
Overall change 541 B 541 B
Edge SSR bundle Size
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
edge-ssr.js gzip 95.4 kB 95.4 kB N/A
page.js gzip 3.06 kB 3.06 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
middleware-b..fest.js gzip 658 B 658 B
middleware-r..fest.js gzip 156 B 156 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 1.65 kB 1.65 kB
Next Runtimes
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
app-page-exp...dev.js gzip 171 kB 171 kB
app-page-exp..prod.js gzip 98 kB 98 kB
app-page-tur..prod.js gzip 99.8 kB 99.8 kB
app-page-tur..prod.js gzip 94 kB 94 kB
app-page.run...dev.js gzip 145 kB 145 kB
app-page.run..prod.js gzip 92.5 kB 92.5 kB
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 15.4 kB 15.4 kB
app-route-tu..prod.js gzip 15.4 kB 15.4 kB
app-route-tu..prod.js gzip 15.2 kB 15.2 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 15.2 kB 15.2 kB
pages-api-tu..prod.js gzip 9.58 kB 9.58 kB
pages-api.ru...dev.js gzip 9.85 kB 9.85 kB
pages-api.ru..prod.js gzip 9.57 kB 9.57 kB
pages-turbo...prod.js gzip 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.2 kB 23.2 kB
pages.runtim..prod.js gzip 22.5 kB 22.5 kB
server.runti..prod.js gzip 51.5 kB 51.5 kB
Overall change 954 kB 954 kB
build cache
vercel/next.js 14-2-1 vercel/next.js revert-server-actions-opt Change
0.pack gzip 1.61 MB 1.6 MB N/A
index.pack gzip 113 kB 113 kB N/A
Overall change 0 B 0 B
Diff details
Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Commit: dfe78ad

@huozhi huozhi marked this pull request as ready for review September 10, 2024 15:38
@huozhi huozhi requested review from ztanner, shuding and ijjk September 10, 2024 15:38
@huozhi huozhi merged commit 99de057 into 14-2-1 Sep 10, 2024
53 of 58 checks passed
@huozhi huozhi deleted the revert-server-actions-opt branch September 10, 2024 19:27
kodiakhq bot referenced this pull request in X-oss-byte/Canary-nextjs Sep 12, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@next/env](https://github.com/vercel/next.js) ([source](https://github.com/vercel/next.js/tree/HEAD/packages/next-env)) | [`14.2.9` -> `14.2.10`](https://renovatebot.com/diffs/npm/@next%2fenv/14.2.9/14.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@next%2fenv/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@next%2fenv/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@next%2fenv/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@next%2fenv/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) |
| [@next/eslint-plugin-next](https://github.com/vercel/next.js) ([source](https://github.com/vercel/next.js/tree/HEAD/packages/eslint-plugin-next)) | [`14.2.9` -> `14.2.10`](https://renovatebot.com/diffs/npm/@next%2feslint-plugin-next/14.2.9/14.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@next%2feslint-plugin-next/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@next%2feslint-plugin-next/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@next%2feslint-plugin-next/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@next%2feslint-plugin-next/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) |
| [@next/polyfill-module](https://github.com/vercel/next.js) ([source](https://github.com/vercel/next.js/tree/HEAD/packages/next-polyfill-module)) | [`14.2.9` -> `14.2.10`](https://renovatebot.com/diffs/npm/@next%2fpolyfill-module/14.2.9/14.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@next%2fpolyfill-module/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@next%2fpolyfill-module/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@next%2fpolyfill-module/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@next%2fpolyfill-module/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) |
| [@next/polyfill-nomodule](https://github.com/vercel/next.js) ([source](https://github.com/vercel/next.js/tree/HEAD/packages/next-polyfill-nomodule)) | [`14.2.9` -> `14.2.10`](https://renovatebot.com/diffs/npm/@next%2fpolyfill-nomodule/14.2.9/14.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@next%2fpolyfill-nomodule/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@next%2fpolyfill-nomodule/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@next%2fpolyfill-nomodule/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@next%2fpolyfill-nomodule/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) |
| [@next/react-refresh-utils](https://github.com/vercel/next.js) ([source](https://github.com/vercel/next.js/tree/HEAD/packages/react-refresh-utils)) | [`14.2.9` -> `14.2.10`](https://renovatebot.com/diffs/npm/@next%2freact-refresh-utils/14.2.9/14.2.10) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@next%2freact-refresh-utils/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@next%2freact-refresh-utils/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@next%2freact-refresh-utils/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@next%2freact-refresh-utils/14.2.9/14.2.10?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency Dashboard for more information.

---

### Release Notes

<details>
<summary>vercel/next.js (@&#8203;next/env)</summary>

### [`v14.2.10`](https://github.com/vercel/next.js/compare/v14.2.9...v14.2.10)

[Compare Source](https://github.com/vercel/next.js/compare/v14.2.9...v14.2.10)

</details>

<details>
<summary>vercel/next.js (@&#8203;next/eslint-plugin-next)</summary>

### [`v14.2.10`](https://github.com/vercel/next.js/compare/v14.2.9...v14.2.10)

[Compare Source](https://github.com/vercel/next.js/compare/v14.2.9...v14.2.10)

</details>

<details>
<summary>vercel/next.js (@&#8203;next/polyfill-module)</summary>

### [`v14.2.10`](https://github.com/vercel/next.js/releases/tag/v14.2.10)

[Compare Source](https://github.com/vercel/next.js/compare/v14.2.9...v14.2.10)

> \[!NOTE]\
> This release is backporting bug fixes. It does **not** include all pending features/changes on canary.

##### Core Changes

-   Remove invalid fallback revalidate value ([https://github.com/vercel/next.js/pull/69990](https://github.com/vercel/next.js/pull/69990))
-   Revert server action optimization ([https://github.com/vercel/next.js/pull/69925](https://github.com/vercel/next.js/pull/69925))
-   Add ability to customize Cache-Control ([#&#8203;69802](https://github.com/vercel/next.js/issues/69802))

##### Credits

Huge thanks to  [@&#8203;huozhi](https://github.com/huozhi) and [@&#8203;ijjk](https://github.com/ijjk) for helping!

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/X-oss-byte/Canary-nextjs).
@TheDevMinerTV
Copy link

TheDevMinerTV commented Sep 12, 2024

So, the convenience of not having to split a file is more important than the security of not leaking actions?

@huozhi
Copy link
Member Author

huozhi commented Sep 12, 2024

@TheDevMinerTV We'll investigate another solution and will fix on canary first, once it's stable we can backport again. On the other side we give better error messaging so they users can get hint to split the modules if possible, and documentation to improve the existign examples.

@TheDevMinerTV
Copy link

TheDevMinerTV commented Sep 12, 2024

I'd rather have the convenience of it just working securely as expected instead of getting compiler warnings / errors for actions. Just disable top-level magic strings.

(or, y'know, remove the magic strings entirely and replace it with a function that you pass your own action functions into).

@FairyPenguin
Copy link

@TheDevMinerTV We'll investigate another solution and will fix on canary first, once it's stable we can backport again. On the other side we give better error messaging so they users can get hint to split the modules if possible, and documentation to improve the existign examples.

This a very strange logic ! so while you are "investigate another solution" it was better to leave the fix for now and when there is a better stable solution replace the existing with the new one! Not remove the fix at all and leave a security issue just because it's not the optimal solution, And "better error messaging" is not a solution for a security issue.

God help us if this is the main mindset for every action in this framework.

I still remember the answer to the slowest ever dev server that it's the users problem not nextjs!

@controversial
Copy link
Contributor

@FairyPenguin An incorrect implementation of tree-shaking server actions caused breaking changes in production apps on a semver-patch release.

The “security issue” of “leaking” actions is really a developer experience issue, where users misunderstand the documented behavior of server actions (to create an endpoint for functions that are exported from a file explicitly marked for server actions).

DX improvements to prevent footguns are a “nice to have,” and cannot come at the cost of production-breaking framework bugs, which is why this feature was reverted. @huozhi has said the team intends to work on bringing the feature back once there’s time to implement it without unexpected breaking changes, which is the correct approach.

In the meantime, if the security model of your app depends on explicitly creating endpoints which you never use and which must never be called, you should either (1) pin [email protected] or (2) refactor your app to avoid exposing insecure endpoints without depending on tree shaking. Being rude to hard-working OSS framework authors is not a solution.

@FairyPenguin
Copy link

@FairyPenguin An incorrect implementation of tree-shaking server actions caused breaking changes in production apps on a semver-patch release.

The “security issue” of “leaking” actions is really a developer experience issue, where users misunderstand the documented behavior of server actions (to create an endpoint for functions that are exported from a file explicitly marked for server actions).

DX improvements to prevent footguns are a “nice to have,” and cannot come at the cost of production-breaking framework bugs, which is why this feature was reverted. @huozhi has said the team intends to work on bringing the feature back once there’s time to implement it without unexpected breaking changes, which is the correct approach.

In the meantime, if the security model of your app depends on explicitly creating endpoints which you never use and which must never be called, you should either (1) pin [email protected] or (2) refactor your app to avoid exposing insecure endpoints without depending on tree shaking. Being rude to hard-working OSS framework authors is not a solution.

You are not embarrassed of yourself ?!!

Your reply needs not just to be a screenshot for learning how to do every logical fallacy and manipulate the words of others and finally play the victim but it's a lesson for how much far people can go behind the words of contributor and opensource to justify the right from the wrong and enforce a lie and wrong technical decisions in a very clear situation that doesn't need any Justifications, fabrications, and twisting and turning!

When there was a memory leak in framework and many dev includes me reported that and you replied no no no memory leak and at the end turns there is a memory leak and was fixed !! and the slowness of the dev server and i lost count !!

If you are not embarrassing yourself you need to take care of your mental health very well.

@STNeto1
Copy link

STNeto1 commented Sep 12, 2024

The problem could have been solved long time ago if they acknowledged the issue before hand, not acting like was a quirk and not a security issue. When the main issue first appeared, took weeks and clout for it to be really seen, ack'd and fixed. Now reverting back to a known issue just because "the dx of the other app" is worse is such a weird move.

@FairyPenguin
Copy link

The problem could have been solved long time ago if they acknowledged the issue before hand, not acting like was a quirk and not a security issue. When the main issue first appeared, took weeks and clout for it to be really seen, ack'd and fixed. Now reverting back to a known issue just because "the dx of the other app" is worse is such a weird move.

This happened exactly in every issue was reported i lost the count

1- The memory leak issue

2- the slow dev server

3- the problem with sharp

4- the server actions

...... I really can't remember how many times it the same attitude and playing the victim and blame the developers for each wrong technical decision they made or an issue appears.

@STNeto1
Copy link

STNeto1 commented Sep 12, 2024

@FairyPenguin An incorrect implementation of tree-shaking server actions caused breaking changes in production apps on a semver-patch release.

The “security issue” of “leaking” actions is really a developer experience issue, where users misunderstand the documented behavior of server actions (to create an endpoint for functions that are exported from a file explicitly marked for server actions).

DX improvements to prevent footguns are a “nice to have,” and cannot come at the cost of production-breaking framework bugs, which is why this feature was reverted. @huozhi has said the team intends to work on bringing the feature back once there’s time to implement it without unexpected breaking changes, which is the correct approach.

In the meantime, if the security model of your app depends on explicitly creating endpoints which you never use and which must never be called, you should either (1) pin [email protected] or (2) refactor your app to avoid exposing insecure endpoints without depending on tree shaking. Being rude to hard-working OSS framework authors is not a solution.

"The “security issue” of “leaking” actions is really a developer experience issue"

The first paragraph from Authentication and authorization says:
"You should treat Server Actions as you would public-facing API endpoints, and ensure that the user is authorized to perform the action. For example:"

The root issue lies on the fact that every async function exported from the file with top level use server , but that's nowere to be found on the page. "You should treat Server Actions as you would public-facing API endpoints" because everything is exported, be used or not, and the even crazier part is that easily findable looking at the transpiled js file.

Acting as if it wasn't an issue is just lolz.

image

@vercel vercel locked as too heated and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants