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

Allow as const syntax in tests #2733

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Jan 6, 2020

looking into why this breaks the build step

Summary

Unblocks #2717

Upgrades the babel packages to their latest versions, allowing for our jest & build setup to support as const syntax. Two side effects to the TS->proptypes plugin:

  • empty exports are now removed from generated code
  • an internal change to the babel typescript preset changed when it looks up the isTSX flag, which broke the TS->proptypes forceTSXParsing code. Instead, babel is now configured to always treat ts files as tsx.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation examples

  • Added or updated jest tests~
  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
    - [ ] A changelog entry exists and is marked appropriately

@chandlerprall chandlerprall removed the request for review from thompsongl January 6, 2020 20:05
@chandlerprall chandlerprall changed the title Allow as const syntax in tests [WIP] Allow as const syntax in tests Jan 6, 2020
@chandlerprall chandlerprall mentioned this pull request Jan 10, 2020
9 tasks
@chandlerprall chandlerprall changed the title [WIP] Allow as const syntax in tests Allow as const syntax in tests Jan 17, 2020
"modules": process.env.BABEL_MODULES ? process.env.BABEL_MODULES : "commonjs" // babel's default is commonjs
}],
"@babel/typescript",
["@babel/typescript", { isTSX: true, allExtensions: true }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no longer a "good" way to hack this functionality into the ts->proptypes generation.

@@ -14,9 +14,10 @@ module.exports = {
]
},
"useBuiltIns": process.env.NO_COREJS_POLYFILL ? false : "usage",
"corejs": "2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New requirement for babel's env preset so they can maintain compatibility in the future when the default version is changed to 3.

@@ -9,10 +9,10 @@
"docker_image": "node:10",
"sideEffects": false,
"scripts": {
"start": "webpack-dev-server --port 8030 --inline --hot --config=src-docs/webpack.config.js",
"start": "BABEL_MODULES=false webpack-dev-server --port 8030 --inline --hot --config=src-docs/webpack.config.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the builds require BABEL_MODULES=false except the lib target. Babel now converts dynamic import() statements to commonjs require statements, which webpack doesn't fully comprehend and is over-eager in what types of files to prepare.

@@ -56,7 +56,7 @@ function handleJSXPath(path) {
function traverseFile(filepath) {
const source = fs.readFileSync(filepath);
const ast = babel.parse(
source,
source.toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buffers are no longer supported

@@ -41,7 +41,7 @@ const webpackConfig = {
{
test: /\.(js|tsx?)$/,
loaders: useCache(['babel-loader']), // eslint-disable-line react-hooks/rules-of-hooks
exclude: /node_modules/,
exclude: [/node_modules/, /packages\/react-datepicker/],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without excluding, babel's transform of react-datepicker's commonjs format conflicts with webpack processing files as ES Modules and everything breaks down.

@@ -22,5 +22,5 @@ export function useDependentState<T>(
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps);

return [state, setState];
return [state, setState] as const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the whole goal of this PR :)

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the annotations

Successfully tested this out with a local branch that previously failed the test suite due to as const use.

@chandlerprall chandlerprall merged commit d9273d6 into elastic:master Jan 17, 2020
@chandlerprall chandlerprall deleted the allow-as-const-syntax-in-tests branch January 17, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants