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

Adding conformance checks #10314

Merged
merged 24 commits into from
Mar 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
24f5a32
adding tests for rect sync conformance check
prateekbh Jan 27, 2020
eace269
Merge branch 'canary' of https://github.com/azukaru/next.js into reac…
prateekbh Jan 27, 2020
87d65d5
adding test for react sync script conformance check
prateekbh Jan 28, 2020
6101d67
Merge branch 'canary' of https://github.com/zeit/next.js into react-e…
prateekbh Jan 28, 2020
ca5f511
Merge branch 'canary' into react-external-sync-script-check
prateekbh Jan 29, 2020
a4e4c74
Merge branch 'canary' of https://github.com/zeit/next.js into react-e…
prateekbh Jan 29, 2020
fc08967
reverting yarn lock changes
prateekbh Jan 29, 2020
e038dd7
Merge branch 'react-external-sync-script-check' of https://github.com…
prateekbh Jan 29, 2020
1431256
adding duplicate polyfill conformance
prateekbh Feb 3, 2020
f282a50
bug fixes in dulpicate polyfill conformance check
prateekbh Feb 4, 2020
5414d46
Merge branch 'canary' of https://github.com/zeit/next.js into react-e…
prateekbh Feb 4, 2020
4417fd3
adding settings capability to conformance plugin
prateekbh Feb 18, 2020
59cac37
Merge branch 'canary' of https://github.com/zeit/next.js into conform…
prateekbh Feb 18, 2020
7f536d6
Merge branch 'canary' of https://github.com/zeit/next.js into conform…
prateekbh Feb 19, 2020
59df992
removing minification check from server build
prateekbh Feb 20, 2020
e7fa784
Merge branch 'canary' of https://github.com/zeit/next.js into conform…
prateekbh Feb 24, 2020
93b8023
Merge branch 'canary' of https://github.com/zeit/next.js into react-e…
prateekbh Feb 24, 2020
2a64738
Merge branch 'canary' into react-external-sync-script-check
prateekbh Feb 24, 2020
2454e7f
Merge branch 'react-external-sync-script-check' into conformance-sett…
prateekbh Feb 24, 2020
2c040c2
bug fix
prateekbh Feb 24, 2020
fd0d462
settings for react sync script check
prateekbh Feb 24, 2020
dc436d7
Merge pull request #279 from azukaru/conformance-settings
prateekbh Feb 27, 2020
7f57a45
Merge branch 'canary' into react-external-sync-script-check
prateekbh Feb 27, 2020
1837668
Merge branch 'canary' into react-external-sync-script-check
prateekbh Mar 2, 2020
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
41 changes: 40 additions & 1 deletion packages/next/build/webpack-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ import { ServerlessPlugin } from './webpack/plugins/serverless-plugin'
import { TerserPlugin } from './webpack/plugins/terser-webpack-plugin/src/index'
import WebpackConformancePlugin, {
MinificationConformanceCheck,
ReactSyncScriptsConformanceCheck,
DuplicatePolyfillsConformanceCheck,
} from './webpack/plugins/webpack-conformance-plugin'

type ExcludesFalse = <T>(x: T | false) => x is T
Expand Down Expand Up @@ -425,6 +427,26 @@ export default async function getBaseWebpackConfig(
customAppFile = path.resolve(path.join(pagesDir, customAppFile))
}

const conformanceConfig = Object.assign(
{
ReactSyncScriptsConformanceCheck: {
enabled: true,
},
MinificationConformanceCheck: {
enabled: true,
},
DuplicatePolyfillsConformanceCheck: {
enabled: true,
BlockedAPIToBePolyfilled: Object.assign(
[],
['fetch'],
config.conformance?.DuplicatePolyfillsConformanceCheck
?.BlockedAPIToBePolyfilled || []
),
},
},
config.conformance
)
let webpackConfig: webpack.Configuration = {
externals: !isServer
? undefined
Expand Down Expand Up @@ -844,7 +866,24 @@ export default async function getBaseWebpackConfig(
config.experimental.conformance &&
!dev &&
new WebpackConformancePlugin({
tests: [new MinificationConformanceCheck()],
tests: [
!isServer &&
conformanceConfig.MinificationConformanceCheck.enabled &&
new MinificationConformanceCheck(),
conformanceConfig.ReactSyncScriptsConformanceCheck.enabled &&
new ReactSyncScriptsConformanceCheck({
AllowedSources:
conformanceConfig.ReactSyncScriptsConformanceCheck
.allowedSources || [],
}),
!isServer &&
conformanceConfig.DuplicatePolyfillsConformanceCheck.enabled &&
new DuplicatePolyfillsConformanceCheck({
BlockedAPIToBePolyfilled:
conformanceConfig.DuplicatePolyfillsConformanceCheck
.BlockedAPIToBePolyfilled,
}),
].filter(Boolean),
}),
].filter((Boolean as any) as ExcludesFalse),
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
import {
IWebpackConformanceTest,
IGetAstNodeResult,
IParsedModuleDetails,
IConformanceTestResult,
IConformanceTestStatus,
} from '../TestInterface'
import {
CONFORMANCE_ERROR_PREFIX,
CONFORMANCE_WARNING_PREFIX,
} from '../constants'
import { types } from 'recast'
import { NodePath } from 'ast-types/lib/node-path'
import { namedTypes } from 'ast-types'
import { getLocalFileName } from '../utils/file-utils'
import {
isNodeCreatingScriptElement,
reducePropsToObject,
} from '../utils/ast-utils'

function getMessage(
property: string,
request: string,
isWarning: Boolean = false
): string {
if (isWarning) {
return `${CONFORMANCE_WARNING_PREFIX}: Found a ${property} polyfill in ${getLocalFileName(
request
)}.`
}
return `${CONFORMANCE_ERROR_PREFIX}: Found a ${property} polyfill in ${getLocalFileName(
request
)}.`
}

export interface DuplicatePolyfillsConformanceTestSettings {
BlockedAPIToBePolyfilled?: string[]
}

const BANNED_LEFT_OBJECT_TYPES = ['Identifier', 'ThisExpression']

export class DuplicatePolyfillsConformanceCheck
implements IWebpackConformanceTest {
private BlockedAPIs: string[] = []
constructor(options: DuplicatePolyfillsConformanceTestSettings = {}) {
this.BlockedAPIs = options.BlockedAPIToBePolyfilled || []
}
public getAstNode(): IGetAstNodeResult[] {
const EARLY_EXIT_SUCCESS_RESULT: IConformanceTestResult = {
result: IConformanceTestStatus.SUCCESS,
}
return [
{
visitor: 'visitAssignmentExpression',
inspectNode: (
path: NodePath<namedTypes.AssignmentExpression>,
{ request }: IParsedModuleDetails
): IConformanceTestResult => {
const { node } = path
const left = node.left as namedTypes.MemberExpression
/**
* We're only interested in code like `foo.fetch = bar;`.
* For anything else we exit with a success.
* Also foo in foo.bar needs to be either Identifier or `this` and not someFunction().fetch;
*/
if (
left.type !== 'MemberExpression' ||
!BANNED_LEFT_OBJECT_TYPES.includes(left.object.type) ||
left.property.type !== 'Identifier'
) {
return EARLY_EXIT_SUCCESS_RESULT
}
if (!this.BlockedAPIs.includes(left.property.name)) {
return EARLY_EXIT_SUCCESS_RESULT
}
/**
* Here we know the code is `foo.(fetch/URL) = something.
* If foo === this/self, fail it immediately.
* check for this.[fetch|URL(...BlockedAPIs)]/ self.[fetch|URL(...BlockedAPIs)]
**/
if (isNodeThisOrSelf(left.object)) {
return {
result: IConformanceTestStatus.FAILED,
warnings: [
{
message: getMessage(left.property.name, request),
},
],
}
}
/**
* we now are sure the code under examination is
* `globalVar.[fetch|URL(...BlockedAPIs)] = something`
**/
const objectName = (left.object as namedTypes.Identifier).name
const allBindings = path.scope.lookup(objectName)
if (!allBindings) {
/**
* we have absolutely no idea where globalVar came from,
* so lets just exit
**/
return EARLY_EXIT_SUCCESS_RESULT
}

try {
const sourcePath = allBindings.bindings[objectName][0]
const originPath = sourcePath.parentPath
const {
node: originNode,
}: { node: namedTypes.VariableDeclarator } = originPath
if (
originNode.type === 'VariableDeclarator' &&
isNodeThisOrSelf(originNode.init)
) {
return {
result: IConformanceTestStatus.FAILED,
warnings: [
{
message: getMessage(left.property.name, request),
},
],
}
}
if (
originPath.name === 'params' &&
originPath.parentPath.firstInStatement()
) {
/**
* We do not know what will be the value of this param at runtime so we just throw a warning.
* ```
* (function(scope){
* ....
* scope.fetch = new Fetch();
* })(.....)
* ```
*/
return {
result: IConformanceTestStatus.FAILED,
warnings: [
{
message: getMessage(left.property.name, request, true),
},
],
}
}
} catch (e) {
return EARLY_EXIT_SUCCESS_RESULT
}

return EARLY_EXIT_SUCCESS_RESULT
},
},
{
visitor: 'visitCallExpression',
inspectNode: (path: NodePath, { request }: IParsedModuleDetails) => {
const { node }: { node: types.namedTypes.CallExpression } = path
if (!node.arguments || node.arguments.length < 2) {
return EARLY_EXIT_SUCCESS_RESULT
}
if (isNodeCreatingScriptElement(node)) {
const propsNode = node
.arguments[1] as types.namedTypes.ObjectExpression
if (!propsNode.properties) {
return EARLY_EXIT_SUCCESS_RESULT
}
const props: {
[key: string]: string
} = reducePropsToObject(propsNode)
if (!('src' in props)) {
return EARLY_EXIT_SUCCESS_RESULT
}
const foundBannedPolyfill = doesScriptLoadBannedAPIfromPolyfillIO(
props.src,
this.BlockedAPIs
)
if (foundBannedPolyfill) {
return {
result: IConformanceTestStatus.FAILED,
warnings: [
{
message: `${CONFORMANCE_WARNING_PREFIX}: Found polyfill.io loading polyfill for ${foundBannedPolyfill}.`,
},
],
}
}
}
return EARLY_EXIT_SUCCESS_RESULT
},
},
]
}
}

function isNodeThisOrSelf(node: any): boolean {
return (
node.type === 'ThisExpression' ||
(node.type === 'Identifier' && node.name === 'self')
)
}

function doesScriptLoadBannedAPIfromPolyfillIO(
source: string,
blockedAPIs: string[]
): string | undefined {
const url = new URL(source)
if (url.hostname === 'polyfill.io' && url.searchParams.has('features')) {
const requestedAPIs = (url.searchParams.get('features') || '').split(',')
return blockedAPIs.find(api => requestedAPIs.includes(api))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,22 @@ import {
IConformanceTestStatus,
} from '../TestInterface'
import { CONFORMANCE_ERROR_PREFIX } from '../constants'
const EARLY_EXIT_RESULT: IConformanceTestResult = {
result: IConformanceTestStatus.SUCCESS,
}

export class MinificationConformanceCheck implements IWebpackConformanceTest {
public buildStared(options: any): IConformanceTestResult {
if (options.output.path.endsWith('/server')) {
return EARLY_EXIT_RESULT
}
// TODO(prateekbh@): Implement warning for using Terser maybe?

if (options.optimization.minimize === false) {
const { optimization } = options
if (
optimization &&
(optimization.minimize !== true ||
(optimization.minimizer && optimization.minimizer.length === 0))
) {
return {
result: IConformanceTestStatus.FAILED,
errors: [
Expand All @@ -19,9 +29,7 @@ export class MinificationConformanceCheck implements IWebpackConformanceTest {
],
}
} else {
return {
result: IConformanceTestStatus.SUCCESS,
}
return EARLY_EXIT_RESULT
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import {
IWebpackConformanceTest,
IGetAstNodeResult,
IParsedModuleDetails,
IConformanceTestResult,
IConformanceTestStatus,
} from '../TestInterface'
import {
CONFORMANCE_ERROR_PREFIX,
CONFORMANCE_WARNING_PREFIX,
} from '../constants'
import { namedTypes } from 'ast-types/'
import { NodePath } from 'ast-types/lib/node-path'
import { getLocalFileName } from '../utils/file-utils'
import { isNodeCreatingScriptElement } from '../utils/ast-utils'
export const ErrorMessage: string = `${CONFORMANCE_ERROR_PREFIX}: A sync script was found in a react module.`
export const WarningMessage: string = `${CONFORMANCE_WARNING_PREFIX}: A sync script was found in a react module.`
export const ErrorDescription = ``
const EARLY_EXIT_SUCCESS_RESULT: IConformanceTestResult = {
result: IConformanceTestStatus.SUCCESS,
}

export interface ReactSyncScriptsConformanceCheckOptions {
AllowedSources?: String[]
}
export class ReactSyncScriptsConformanceCheck
implements IWebpackConformanceTest {
private allowedSources: String[] = []
constructor({
AllowedSources,
}: ReactSyncScriptsConformanceCheckOptions = {}) {
if (AllowedSources) {
this.allowedSources = AllowedSources
}
}

public getAstNode(): IGetAstNodeResult[] {
return [
{
visitor: 'visitCallExpression',
inspectNode: (path: NodePath, { request }: IParsedModuleDetails) => {
const { node }: { node: namedTypes.CallExpression } = path
if (!node.arguments || node.arguments.length < 2) {
return EARLY_EXIT_SUCCESS_RESULT
}
if (isNodeCreatingScriptElement(node)) {
const propsNode = node.arguments[1] as namedTypes.ObjectExpression
if (!propsNode.properties) {
return EARLY_EXIT_SUCCESS_RESULT
}
const props: {
[key: string]: string
} = propsNode.properties.reduce((originalProps, prop: any) => {
// @ts-ignore
originalProps[prop.key.name] = prop.value.value
return originalProps
}, {})
if (
'defer' in props ||
'async' in props ||
!('src' in props) ||
this.allowedSources.includes(props.src)
) {
return EARLY_EXIT_SUCCESS_RESULT
}

// Todo: Add an absolute error case for modern js when class is a subclass of next/head.
return {
result: IConformanceTestStatus.FAILED,
warnings: [
{
message: `${WarningMessage} ${getLocalFileName(
request
)}. This can potentially delay FCP/FP metrics.`,
},
],
}
}
return EARLY_EXIT_SUCCESS_RESULT
},
},
]
}
}
Loading