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

Experimental Next.js 12 Output File Tracing #2169

Merged
merged 4 commits into from
Jan 27, 2022
Merged

Experimental Next.js 12 Output File Tracing #2169

merged 4 commits into from
Jan 27, 2022

Conversation

iiroj
Copy link
Contributor

@iiroj iiroj commented Dec 3, 2021

This PR implements the Next.js 12 Output File Tracing, meaning that @sls-next/serverless-component will no longer set any target while building Next.js, and will instead use the generated *.nft.json files to copy required files into the Lambda@Edge deployment.

To use the new feature, one should:

  • update to Next.js 12
  • make sure there is no target in the Next.js configuration (it's now deprecated)
  • set the build.outputFileTracing: true in the serverless.yml inputs for @sls-next/serverless-component

@iiroj
Copy link
Contributor Author

iiroj commented Dec 3, 2021

Ping @dphang I replaced #2124 with this.

@slsnextbot
Copy link
Collaborator

slsnextbot commented Dec 3, 2021

Handler Size Report

No changes to handler sizes.

Base Handler Sizes (kB) (commit e492064)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1524,
            "Minified": 668
        },
        "Image Lambda": {
            "Standard": 1488,
            "Minified": 800
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1534,
            "Minified": 673
        },
        "Default Lambda V2": {
            "Standard": 1526,
            "Minified": 670
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1496,
            "Minified": 805
        },
        "Regeneration Lambda": {
            "Standard": 1187,
            "Minified": 546
        },
        "Regeneration Lambda V2": {
            "Standard": 1253,
            "Minified": 573
        }
    }
}

New Handler Sizes (kB) (commit a0b9240)

{
    "Lambda": {
        "Default Lambda": {
            "Standard": 1524,
            "Minified": 668
        },
        "Image Lambda": {
            "Standard": 1488,
            "Minified": 800
        }
    },
    "Lambda@Edge": {
        "Default Lambda": {
            "Standard": 1534,
            "Minified": 673
        },
        "Default Lambda V2": {
            "Standard": 1526,
            "Minified": 670
        },
        "API Lambda": {
            "Standard": 634,
            "Minified": 318
        },
        "Image Lambda": {
            "Standard": 1496,
            "Minified": 805
        },
        "Regeneration Lambda": {
            "Standard": 1187,
            "Minified": 546
        },
        "Regeneration Lambda V2": {
            "Standard": 1253,
            "Minified": 573
        }
    }
}

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #2169 (80c8989) into master (e492064) will decrease coverage by 0.06%.
The diff coverage is 91.39%.

❗ Current head 80c8989 differs from pull request most recent head a0b9240. Consider uploading reports for the commit a0b9240 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2169      +/-   ##
==========================================
- Coverage   83.54%   83.48%   -0.07%     
==========================================
  Files         102      104       +2     
  Lines        3671     3711      +40     
  Branches     1169     1189      +20     
==========================================
+ Hits         3067     3098      +31     
- Misses        592      601       +9     
  Partials       12       12              
Impacted Files Coverage Δ
...rless-components/nextjs-component/src/component.ts 86.15% <75.00%> (-0.21%) ⬇️
packages/libs/lambda-at-edge/src/build.ts 92.96% <86.04%> (-3.12%) ⬇️
.../lambda-at-edge/src/lib/copyRequiredServerFiles.ts 92.85% <92.85%> (ø)
...ibs/lambda-at-edge/src/lib/copyOutputFileTraces.ts 100.00% <100.00%> (ø)
...ges/libs/lambda-at-edge/src/lib/isPathInsideDir.ts 100.00% <100.00%> (ø)
...ponents/nextjs-component/__tests__/aws-sdk.mock.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e492064...a0b9240. Read the comment docs.

@jonahallibone
Copy link

Is this a no-go? I'd really love to use the new output format

@iiroj
Copy link
Contributor Author

iiroj commented Jan 12, 2022

@jonahallibone I haven't had time to write really exhaustive tests... In the meantime, it seems Next.js is already working on a way to do the output themselves: https://nextjs.org/docs/advanced-features/output-file-tracing#automatically-copying-traced-files-experimental

@Purii
Copy link

Purii commented Jan 18, 2022

Awesome! We absolutely need this feature to include localization files.
Is there anything I can support with or is this PR only waiting for approval + merge by @dphang ?

@iiroj
Copy link
Contributor Author

iiroj commented Jan 18, 2022

@Purii I do want to write more tests because otherwise the codecoverage check doesn't pass, but it does seem to work at least for my small site!

const outputFileTracing =
typeof inputs.build !== "boolean" &&
typeof inputs.build !== "undefined" &&
!!inputs.build.outputFileTracing;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably can cover it in custom inputs test? or if it's not too convenient, we could modify the existing e2e tests to cover this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a custom inputs test and also updated the next-app-experimental to use this flag.

@dphang
Copy link
Collaborator

dphang commented Jan 20, 2022

thanks! I think it looks fine for the most part, codecov drop is just 0.08%, I gave a minor comment, I can probably add a new app to test this later so we get coverage there

dphang
dphang previously approved these changes Jan 24, 2022
@iiroj iiroj changed the title 🚧 Next.js 12 Output File Tracing Experimental Next.js 12 Output File Tracing Jan 24, 2022
@iiroj iiroj requested a review from dphang January 24, 2022 17:06
@iiroj
Copy link
Contributor Author

iiroj commented Jan 25, 2022

@dphang I'm not familiar with the e2e test setup; would it make sense to run those before merging?

@dphang
Copy link
Collaborator

dphang commented Jan 25, 2022

@dphang I'm not familiar with the e2e test setup; would it make sense to run those before merging?

Ran it here https://github.com/serverless-nextjs/serverless-next.js/actions/runs/1744128397 (there are some issues with the experimental app which I will fix later but if it passes on the other apps it should be fine)

@iiroj
Copy link
Contributor Author

iiroj commented Jan 25, 2022

@dphang seems like another e2e app failed, how to debug? I'll try to run this branch locally against my own site if it catches any errors.

@dphang
Copy link
Collaborator

dphang commented Jan 25, 2022

Ya that app is because somehow the IAM policy got corrupted, I need to manually fix it. But it should be fine since actually your changes is primarily build-time (the apps tests are to sanity check it doesn't break existing things) and experimental now so we can add more e2e tests later.

@dphang dphang merged commit e01571f into serverless-nextjs:master Jan 27, 2022
@iiroj iiroj deleted the output-file-tracing branch January 27, 2022 08:11
@iiroj
Copy link
Contributor Author

iiroj commented Jan 27, 2022

@dphang did the tests fail because of this branch? Any way I can help debug them?

https://github.com/serverless-nextjs/serverless-next.js/actions/runs/1755195463

@dphang
Copy link
Collaborator

dphang commented Jan 27, 2022

Hm I think a new minor version of nextjs yesterday is causing failures since we pick the latest one, need to look into logs and fix it but your changes hadn't failed earlier when I ran tests. I will check why it is failing, they may have introduced some new required files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants