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

[optimizer] validate the syntax of bundled node_modules #59972

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Mar 12, 2020

Fixes #59833

In order to avoid including node_modules in the bundles that break IE compatibility (something that has been plaguing us lately) this webpack plugin checks the AST created by webpack for each node_module being included in the dll.

It does this using acorn-walk, a module developed by the authors of the default parser webpack uses, to traverse all the nodes in the AST. The nodes are checked against the DisallowedSyntaxCheck objects defined in src/optimize/dynamic_dll_plugin/disallowed_syntax.ts. These checks are based on the syntax definitions described by https://github.com/estree/estree, the specification for the AST produced by acorn.

Each SyntaxCheck can be triggered for one or more estree.Node['type']. When a node of these types is found and either the check has no test method, or the test method returns true, then we throw an error indicating that the disallowed syntax was discovered in the node_module. This triggers webpack to report the module as failing to parse:

Performance

In order to reduce the performance impact of this check, the validation is performed on the same AST that webpack is already generating. When switching between this branch and master (with an import in place to make sure the DLL needs to be rebuilt) I'm not seeing any meaningful difference in optimizer execution time.

@spalger spalger changed the title [optimizer] validate the syntax of node_modules included in the dll [optimizer] validate the syntax of bundled node_modules Mar 12, 2020
@spalger spalger marked this pull request as ready for review March 27, 2020 22:41
@spalger spalger requested review from a team as code owners March 27, 2020 22:41
@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v7.8.0 v8.0.0 labels Mar 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for src/legacy/server/config/schema.js changes

Comment on lines +28 to +36
export const checks: DisallowedSyntaxCheck[] = [
/**
* es2015
*/
// https://github.com/estree/estree/blob/master/es2015.md#functions
{
name: '[es2015] generator function',
nodeType: ['FunctionDeclaration', 'FunctionExpression'],
test: (n: estree.FunctionDeclaration | estree.FunctionExpression) => !!n.generator,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be surprised if we were to be the first project to have such needs. Isn't there no oss tools performing such checks atm?

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 looked around and didn't find anything, but I would be shocked if we could find something that would integrate with the webpack parser so tightly in order to avoid parsing the bundles all over again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, from a quick glance, every webpack plugin integration (ie https://github.com/bitjourney/check-es-version-webpack-plugin/blob/master/index.js) are also using acorn at the end and parses the whole files again. None are doing the fine-grained checks you implemented though.

@spalger
Copy link
Contributor Author

spalger commented Mar 30, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

LGTM, well done.

@spalger spalger merged commit 7d58f95 into elastic:master Mar 30, 2020
spalger pushed a commit to spalger/kibana that referenced this pull request Mar 30, 2020
Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	renovate.json5
spalger pushed a commit to spalger/kibana that referenced this pull request Mar 30, 2020
Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	renovate.json5
@spalger
Copy link
Contributor Author

spalger commented Mar 31, 2020

7.x/7.8: 6065f58
7.7: 7321d5e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Look for invalid syntax in bundles in build process
5 participants