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 amphtml link rel not respecting basePath #15949

Merged
merged 4 commits into from
Aug 8, 2020
Merged

Fix amphtml link rel not respecting basePath #15949

merged 4 commits into from
Aug 8, 2020

Conversation

amiralies
Copy link
Contributor

Closes #15244

@ijjk ijjk added the type: next label Aug 6, 2020
@ijjk
Copy link
Member

ijjk commented Aug 6, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
buildDuration 9.2s 9.3s ⚠️ +56ms
nodeModulesSize 65.9 MB 65.9 MB ⚠️ +316 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
/ failed reqs 0 0 ✓
/ total time (seconds) 1.686 1.698 ⚠️ +0.01
/ avg req/sec 1482.87 1472.15 ⚠️ -10.72
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 0.909 0.944 ⚠️ +0.03
/error-in-render avg req/sec 2749.7 2649.31 ⚠️ -100.39
Client Bundles (main, webpack, commons)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..795d.js gzip 9.99 kB 9.99 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-c30ac70..0d39.js gzip 6.74 kB 6.74 kB ✓
webpack-ccf5..276a.js gzip 751 B 751 B ✓
Overall change 56.6 kB 56.6 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..dule.js gzip 5.89 kB 5.89 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-61da9c7..dule.js gzip 5.81 kB 5.81 kB ✓
webpack-10c7..dule.js gzip 751 B 751 B ✓
Overall change 51.6 kB 51.6 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
polyfills-75..1629.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB ✓
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB ✓
hooks-585f07..95a3.js gzip 887 B 887 B ✓
index-c7b63f..fc02.js gzip 227 B 227 B ✓
link-f4d2979..e57b.js gzip 1.29 kB 1.29 kB ✓
routerDirect..ebc7.js gzip 284 B 284 B ✓
withRouter-2..db68.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-97e743e..dule.js gzip 626 B 626 B ✓
_error-b4004..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-696209..dule.js gzip 387 B 387 B ✓
index-a4dd74..dule.js gzip 226 B 226 B ✓
link-653c74f..dule.js gzip 1.26 kB 1.26 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-1..dule.js gzip 282 B 282 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_buildManifest.js gzip 273 B 273 B ✓
_buildManife..dule.js gzip 280 B 280 B ✓
Overall change 553 B 553 B ✓
Rendered Page Sizes
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
index.html gzip 946 B 946 B ✓
link.html gzip 953 B 953 B ✓
withRouter.html gzip 940 B 940 B ✓
Overall change 2.84 kB 2.84 kB ✓
Serverless Mode
General Overall increase ⚠️
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
buildDuration 10.5s 10.1s -415ms
nodeModulesSize 65.9 MB 65.9 MB ⚠️ +316 B
Client Bundles (main, webpack, commons)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..795d.js gzip 9.99 kB 9.99 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-c30ac70..0d39.js gzip 6.74 kB 6.74 kB ✓
webpack-ccf5..276a.js gzip 751 B 751 B ✓
Overall change 56.6 kB 56.6 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..dule.js gzip 5.89 kB 5.89 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-61da9c7..dule.js gzip 5.81 kB 5.81 kB ✓
webpack-10c7..dule.js gzip 751 B 751 B ✓
Overall change 51.6 kB 51.6 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
polyfills-75..1629.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB ✓
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB ✓
hooks-585f07..95a3.js gzip 887 B 887 B ✓
index-c7b63f..fc02.js gzip 227 B 227 B ✓
link-f4d2979..e57b.js gzip 1.29 kB 1.29 kB ✓
routerDirect..ebc7.js gzip 284 B 284 B ✓
withRouter-2..db68.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-97e743e..dule.js gzip 626 B 626 B ✓
_error-b4004..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-696209..dule.js gzip 387 B 387 B ✓
index-a4dd74..dule.js gzip 226 B 226 B ✓
link-653c74f..dule.js gzip 1.26 kB 1.26 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-1..dule.js gzip 282 B 282 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_buildManifest.js gzip 273 B 273 B ✓
_buildManife..dule.js gzip 280 B 280 B ✓
Overall change 553 B 553 B ✓
Serverless bundles
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_error.js 1.03 MB 1.03 MB ✓
404.html 4.18 kB 4.18 kB ✓
hooks.html 3.82 kB 3.82 kB ✓
index.js 1.03 MB 1.03 MB ✓
link.js 1.07 MB 1.07 MB ✓
routerDirect.js 1.06 MB 1.06 MB ✓
withRouter.js 1.06 MB 1.06 MB ✓
Overall change 5.26 MB 5.26 MB ✓
Commit: f9ea2ec

Timer
Timer previously requested changes Aug 6, 2020
View reviewed changes
Copy link
Member

@Timer Timer left a comment

Choose a reason for hiding this comment

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

Please add a test for this behavior!

timneutkens
timneutkens requested changes Aug 7, 2020
View reviewed changes
Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

This needs tests 👍

@amiralies
Copy link
Contributor Author

I've tried add a test for this, I'm not familiar with code base though, correct me if my approach is wrong
cc/ @Timer @timneutkens

@ijjk
Copy link
Member

ijjk commented Aug 7, 2020

Stats from current PR

Default Server Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
buildDuration 13.4s 13.6s ⚠️ +178ms
nodeModulesSize 66 MB 66 MB ⚠️ +316 B
Page Load Tests Overall increase ✓
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
/ failed reqs 0 0 ✓
/ total time (seconds) 2.509 2.472 -0.04
/ avg req/sec 996.25 1011.42 +15.17
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.548 1.533 -0.02
/error-in-render avg req/sec 1614.63 1630.73 +16.1
Client Bundles (main, webpack, commons)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..ec1a.js gzip 9.99 kB 9.99 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-008324e..83e7.js gzip 6.74 kB 6.74 kB ✓
webpack-ccf5..276a.js gzip 751 B 751 B ✓
Overall change 56.6 kB 56.6 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..dule.js gzip 5.89 kB 5.89 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-c621fe2..dule.js gzip 5.81 kB 5.81 kB ✓
webpack-10c7..dule.js gzip 751 B 751 B ✓
Overall change 51.6 kB 51.6 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
polyfills-75..1629.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB ✓
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB ✓
hooks-585f07..95a3.js gzip 887 B 887 B ✓
index-c7b63f..fc02.js gzip 227 B 227 B ✓
link-f4d2979..e57b.js gzip 1.29 kB 1.29 kB ✓
routerDirect..ebc7.js gzip 284 B 284 B ✓
withRouter-2..db68.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-97e743e..dule.js gzip 626 B 626 B ✓
_error-b4004..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-696209..dule.js gzip 387 B 387 B ✓
index-a4dd74..dule.js gzip 226 B 226 B ✓
link-653c74f..dule.js gzip 1.26 kB 1.26 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-1..dule.js gzip 282 B 282 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_buildManifest.js gzip 273 B 273 B ✓
_buildManife..dule.js gzip 280 B 280 B ✓
Overall change 553 B 553 B ✓
Rendered Page Sizes
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
index.html gzip 948 B 948 B ✓
link.html gzip 954 B 954 B ✓
withRouter.html gzip 941 B 941 B ✓
Overall change 2.84 kB 2.84 kB ✓
Serverless Mode
General Overall increase ⚠️
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
buildDuration 14.8s 14.9s ⚠️ +97ms
nodeModulesSize 66 MB 66 MB ⚠️ +316 B
Client Bundles (main, webpack, commons)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..ec1a.js gzip 9.99 kB 9.99 kB ✓
framework.HASH.js gzip 39.1 kB 39.1 kB ✓
main-008324e..83e7.js gzip 6.74 kB 6.74 kB ✓
webpack-ccf5..276a.js gzip 751 B 751 B ✓
Overall change 56.6 kB 56.6 kB ✓
Client Bundles (main, webpack, commons) Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
677f882d2ed8..dule.js gzip 5.89 kB 5.89 kB ✓
framework.HA..dule.js gzip 39.1 kB 39.1 kB ✓
main-c621fe2..dule.js gzip 5.81 kB 5.81 kB ✓
webpack-10c7..dule.js gzip 751 B 751 B ✓
Overall change 51.6 kB 51.6 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
polyfills-75..1629.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-874bd8a..0103.js gzip 1.28 kB 1.28 kB ✓
_error-fa39c..ec40.js gzip 3.45 kB 3.45 kB ✓
hooks-585f07..95a3.js gzip 887 B 887 B ✓
index-c7b63f..fc02.js gzip 227 B 227 B ✓
link-f4d2979..e57b.js gzip 1.29 kB 1.29 kB ✓
routerDirect..ebc7.js gzip 284 B 284 B ✓
withRouter-2..db68.js gzip 284 B 284 B ✓
Overall change 7.71 kB 7.71 kB ✓
Client Pages Modern
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_app-97e743e..dule.js gzip 626 B 626 B ✓
_error-b4004..dule.js gzip 2.3 kB 2.3 kB ✓
hooks-696209..dule.js gzip 387 B 387 B ✓
index-a4dd74..dule.js gzip 226 B 226 B ✓
link-653c74f..dule.js gzip 1.26 kB 1.26 kB ✓
routerDirect..dule.js gzip 284 B 284 B ✓
withRouter-1..dule.js gzip 282 B 282 B ✓
Overall change 5.37 kB 5.37 kB ✓
Client Build Manifests
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_buildManifest.js gzip 273 B 273 B ✓
_buildManife..dule.js gzip 280 B 280 B ✓
Overall change 553 B 553 B ✓
Serverless bundles
vercel/next.js canary amiralies/next.js fix-amp-rel-basepath Change
_error.js 1.03 MB 1.03 MB ✓
404.html 4.18 kB 4.18 kB ✓
hooks.html 3.82 kB 3.82 kB ✓
index.js 1.03 MB 1.03 MB ✓
link.js 1.07 MB 1.07 MB ✓
routerDirect.js 1.06 MB 1.06 MB ✓
withRouter.js 1.06 MB 1.06 MB ✓
Overall change 5.26 MB 5.26 MB ✓
Commit: ecf3e25

@timneutkens
Copy link
Member

Looks great @amiralies, awesome work 🙏

@ijjk ijjk dismissed Timer’s stale review August 8, 2020 13:07

Tests added

@timneutkens timneutkens merged commit 4b6e9a4 into vercel:canary Aug 8, 2020
@vercel vercel locked as resolved and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ijjk ijjk ijjk approved these changes

@timneutkens timneutkens timneutkens approved these changes

@Timer Timer Timer left review comments

@lfades lfades Awaiting requested review from lfades

Assignees
No one assigned
Labels
type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Respect basePath in amphtml link rel
4 participants
@amiralies @ijjk @timneutkens @Timer