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.

Sign up for GitHub

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

Jump to bottom

fix: 🐛 include terser bundle into netlify functions #1295

Merged
merged 5 commits into from
Apr 4, 2022
Merged

fix: 🐛 include terser bundle into netlify functions #1295

merged 5 commits into from
Apr 4, 2022

Conversation

ericapisani
Copy link

Resolves issues where AMP would work locally, but not on a deployed site.

Summary

Someone had reported an issue where they were able to see that pages configured for AMP were working in their local environments, but not when they were deployed to Netlify.

In testing, it appeared that the terser package was needed but was being omitted from the netlify function .zip file generated at the onBuild step.

Removing the line that omitted the terser bundle resolves the issue.

Test plan

  1. Visit the Deploy Preview

The test deploy site has the home page configured as a hybrid AMP page. This means that you can test both the non-AMP and AMP views.

AMP URL: https://624b075121937b095892dd5c--amp-nextjs-investigation-with-ci.netlify.app?amp=1
Non-AMP URL: https://624b075121937b095892dd5c--amp-nextjs-investigation-with-ci.netlify.app/

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Google AMP Test Results for deploy site: https://search.google.com/test/amp/result?id=4llAXD5fMCWJiKavPHv_mg

baby duck

Standard checks:

  • Check the Deploy Preview's Demo site for your PR's functionality

@ericapisani
fix: 🐛 include terser bundle into netlify functions
921eb07 
Resolves issues where AMP would work locally, but not on a deployed site
@ericapisani ericapisani requested a review from a team April 4, 2022 15:24
@ericapisani ericapisani self-assigned this Apr 4, 2022
@netlify
Copy link

netlify bot commented Apr 4, 2022 •
edited
Loading

✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!

Name Link
🔨 Latest commit 188fdba
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/624b3f5cffc02600092ec64a
😎 Deploy Preview https://deploy-preview-1295--netlify-plugin-nextjs-nx-monorepo-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 4, 2022 •
edited
Loading

✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!

Name Link
🔨 Latest commit 188fdba
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-static-root-demo/deploys/624b3f5ce6bd45000989061b
😎 Deploy Preview https://deploy-preview-1295--netlify-plugin-nextjs-static-root-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 4, 2022 •
edited
Loading

✅ Deploy Preview for netlify-plugin-nextjs-demo ready!

Name Link
🔨 Latest commit 188fdba
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/624b3f5c339d9600092abd98
😎 Deploy Preview https://deploy-preview-1295--netlify-plugin-nextjs-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Apr 4, 2022
@netlify
Copy link

netlify bot commented Apr 4, 2022 •
edited
Loading

✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!

Name Link
🔨 Latest commit 188fdba
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/624b3f5c98c2810008626d98
😎 Deploy Preview https://deploy-preview-1295--netlify-plugin-nextjs-export-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cypress
Copy link

cypress bot commented Apr 4, 2022 •
edited
Loading


Test summary

2 • 0 • 0 • 0

Run details
Project netlify-plugin-nextjs-nx-monorepo-demo
Status Passed
Commit 305b9cf ℹ️
Started Apr 4, 2022 6:59 PM
Ended Apr 4, 2022 7:01 PM
Duration 01:13 💡
OS Linux Ubuntu - 20.04
Browser Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@cypress
Copy link

cypress bot commented Apr 4, 2022 •
edited
Loading


Test summary

22 • 0 • 0 • 0

Run details
Project netlify-plugin-nextjs-default-demo
Status Passed
Commit 305b9cf ℹ️
Started Apr 4, 2022 6:59 PM
Ended Apr 4, 2022 7:01 PM
Duration 01:29 💡
OS Linux Ubuntu - 20.04
Browser Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@cypress
Copy link

cypress bot commented Apr 4, 2022 •
edited
Loading


Test summary

7 • 0 • 0 • 0

Run details
Project netlify-plugin-nextjs-static-demo
Status Passed
Commit 305b9cf ℹ️
Started Apr 4, 2022 7:01 PM
Ended Apr 4, 2022 7:02 PM
Duration 01:29 💡
OS Linux Ubuntu - 20.04
Browser Chrome 100

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@ericapisani
Copy link
Author

The failing unit tests look to be due to #1289 needing to be merged first (newer nextjs version having moved the type that's imported within the src/helpers/functions file)

ascorbic
ascorbic previously approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Looking great, but let's get #1289 merged before it so the tests work properly

@ascorbic
Copy link
Contributor

ascorbic commented Apr 4, 2022

Oh, like you said!

@kodiakhq kodiakhq bot merged commit f29adf3 into main Apr 4, 2022
@kodiakhq kodiakhq bot deleted the ep-include-terser-in-function-zip branch April 4, 2022 21:10
@token-generator-app token-generator-app bot mentioned this pull request Apr 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ascorbic ascorbic ascorbic approved these changes

Assignees

@ericapisani ericapisani

Labels
automerge type: bug code to address defects in shipped code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericapisani @ascorbic