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

Ensure validator is included in vendored AMP validator #70482

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Ensure validator is included in vendored AMP validator #70482

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d144419
Ensure validator is included in vendored AMP validator
ijjk Sep 25, 2024
fed17db
download validator separate of ncc
ijjk Sep 25, 2024
5c9a322
fix
ijjk Sep 25, 2024
bbd2fd0
remove old test file
ijjk Sep 25, 2024
5074bff
remove old note
ijjk Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Ensure validator is included in vendored AMP validator
  • Loading branch information
@ijjk
ijjk committed Sep 25, 2024
commit d14441913628acde7b23e3f9f88581847747e64a
2,247 changes: 2,247 additions & 0 deletions packages/next/src/compiled/amphtml-validator/validator_wasm.js
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions packages/next/src/export/routes/pages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ export async function exportPagesPage(
hybrid: components.pageConfig?.amp === 'hybrid',
}

if (!ampValidatorPath) {
ampValidatorPath = require.resolve(
'next/dist/compiled/amphtml-validator/validator_wasm.js'
)
}

const inAmpMode = isInAmpMode(ampState)
const hybridAmp = ampState.hybrid

Expand Down
10 changes: 7 additions & 3 deletions packages/next/src/server/dev/next-dev-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,13 @@ export default class DevServer extends Server {
this.nextConfig.experimental?.amp?.skipValidation ?? false
this.renderOpts.ampValidator = (html: string, pathname: string) => {
const validatorPath =
this.nextConfig.experimental &&
this.nextConfig.experimental.amp &&
this.nextConfig.experimental.amp.validator
(this.nextConfig.experimental &&
this.nextConfig.experimental.amp &&
this.nextConfig.experimental.amp.validator) ||
require.resolve(
'next/dist/compiled/amphtml-validator/validator_wasm.js'
)

const AmpHtmlValidator =
require('next/dist/compiled/amphtml-validator') as typeof import('next/dist/compiled/amphtml-validator')
return AmpHtmlValidator.getInstance(validatorPath).then((validator) => {
Expand Down
15 changes: 15 additions & 0 deletions packages/next/taskfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,21 @@ export async function ncc_amphtml_validator(task, opts) {
.source(relative(__dirname, require.resolve('amphtml-validator')))
.ncc({ packageName: 'amphtml-validator', externals })
.target('src/compiled/amphtml-validator')

const validatorRes = await fetch(
'https://cdn.ampproject.org/v0/validator_wasm.js'
Copy link
Member

@eps1lon eps1lon Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is that a permalink or does it automatically get new versions? It should be an exact version so that PRs don't start to randomly fail because the vendored source changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the exact URL used by the library https://github.com/ampproject/amphtml/blob/c8bbc75e4274d7ef37d0b218344cfa937020b62b/validator/js/nodejs/index.js#L289-L290

Copy link
Member Author

Choose a reason for hiding this comment

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

We could move it after ncc'ing step so it can change without invalidating the vendored

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

I suspect it still is some form of "get me the latest source" which isn't a problem if you keep it in memory like they do. Makes CI non-reproducible but I also don't have a better suggestion other than moving the vendoring to an ad-hoc task. But that's not compatible with how we clean so 🤷

We definitely have an issue with flaky tests so this is a good step in the right direction.

)

if (!validatorRes.ok) {
throw new Error(
`Failed to get the AMP validator, status: ${validatorRes.status}`
)
}

await fs.writeFile(
join(__dirname, 'src/compiled/amphtml-validator/validator_wasm.js'),
require('buffer').Buffer.from(await validatorRes.arrayBuffer())
)
}

// eslint-disable-next-line camelcase
Expand Down
8 changes: 1 addition & 7 deletions test/e2e/async-modules/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('./amp-validator-wasm.js'),
},
},
}
module.exports = {}
5 changes: 0 additions & 5 deletions test/integration/amp-export-validation/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
module.exports = {
output: 'export',
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
// exportPathMap
}
1 change: 0 additions & 1 deletion test/integration/amphtml-custom-optimizer/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
optimizer: {
ampRuntimeVersion: '001515617716922',
rtv: true,
Expand Down
8 changes: 1 addition & 7 deletions test/integration/amphtml-custom-validator/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
module.exports = {}
8 changes: 1 addition & 7 deletions test/integration/amphtml-fragment-style/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
module.exports = {}
8 changes: 1 addition & 7 deletions test/integration/amphtml-ssg/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
module.exports = {}
5 changes: 0 additions & 5 deletions test/integration/amphtml/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,5 @@ module.exports = {
amp: {
canonicalBase: 'http://localhost:1234',
},
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
// edit here
}
5 changes: 0 additions & 5 deletions test/integration/auto-export-query-error/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,4 @@ module.exports = {
'/ssr': { page: '/ssr', query: { another: 'one' } },
}
},
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
8 changes: 1 addition & 7 deletions test/integration/build-output/fixtures/with-amp/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../../../lib/amp-validator-wasm.js'),
},
},
}
module.exports = {}
5 changes: 0 additions & 5 deletions test/integration/export-default-map/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
module.exports = {
output: 'export',
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
5 changes: 0 additions & 5 deletions test/integration/i18n-support-base-path/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
module.exports = {
basePath: '/docs',
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
i18n: {
// localeDetection: false,
locales: [
Expand Down
5 changes: 0 additions & 5 deletions test/integration/i18n-support/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
module.exports = {
// basePath: '/docs',
// trailingSlash: true,
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
i18n: {
// localeDetection: false,
locales: [
Expand Down
8 changes: 1 addition & 7 deletions test/integration/page-config/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1 @@
module.exports = {
experimental: {
amp: {
validator: require.resolve('../../lib/amp-validator-wasm.js'),
},
},
}
module.exports = {}
5 changes: 0 additions & 5 deletions test/production/pages-dir/production/fixture/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,6 @@
setInterval(() => {}, 250)

module.exports = {
experimental: {
amp: {
validator: require.resolve('./amp-validator-wasm.js'),
},
},
onDemandEntries: {
// Make sure entries are not getting disposed.
maxInactiveAge: 1000 * 60 * 60,
Expand Down
Loading