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

Build targeting Jest with relevant transforms #2698

Merged
merged 24 commits into from
Feb 6, 2020

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Dec 18, 2019

Summary

Adds a third build target: commonjs via babel with async function and dynamic import transforms applied.
The goal is to eliminate needing to transform EUI from within Kibana's Jest setup, using moduleNameMapper to point to the targeted build in testing contexts. See the Draft PR for changes to be make to Kibana.

Resolves #2065

This also proposes a pattern for overriding components ad hoc for the benefit of testing. See changes related to icons.tsx, in which the component is fully rewritten to avoid dynamic imports. Looking for opinions on what we should actually render when it comes to icon snapshots.

Testing steps:

  1. yarn build from eui
  2. Either npm pack and change Kibana's package.json, or copy over a subset of directories to Kibana node_modules
  3. yarn test:jest from kibana
  4. Expected output is a slew of test failures related to icon snapshots

Checklist

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

  • Added documentation examples (document creating *.testenv.* files for testing purposes)
  • 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

.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

  • I think test-kbn would make more sense as test-env or testing-env
  • at this point should all build targets be moved to dist? e.g. dist/es, dist/lib, etc
  • needs documentation for using (creating testenv files) and consuming (using this build in another project) - once things are finalized, but wanted to include it here anyway

src/index.testenv.js Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor Author

I think test-kbn would make more sense as test-env or testing-env

Changed to test-env

at this point should all build targets be moved to dist? e.g. dist/es, dist/lib, etc

I think yes, but I'd prefer to do that at a separate time. The number of import path updates that would be necessary in Kibana alone is very large.

needs documentation for using (creating testenv files) and consuming (using this build in another project) - once things are finalized, but wanted to include it here anyway

For sure. I've updated the description checklist to make this a todo item.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One nit, one real ask.

scripts/compile-eui.js Outdated Show resolved Hide resolved
scripts/compile-eui.js Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One more round, then let's move on to the next phase of this! :)

scripts/compile-eui.js Outdated Show resolved Hide resolved
src/components/icon/icon.testenv.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Left one more thought, but it shouldn't be blocking for getting additional feedback on this.

Pulled & built, verifying:

  • existing es & lib builds are not affected by the mocked icon
  • new test-env build is generated as expected, a commonjs build like lib but has icon.testenv.jsx applied
  • eui.d.ts does not contain definitions from .testenv. files
  • testenv build can be pulled into a JSDOM environment, with ReactDOM correctly rendering the mocked icon to the expected string
  • testenv directory is excluded from git
  • npm pack includes the new test-env target

src/components/icon/icon.testenv.tsx Outdated Show resolved Hide resolved
@thompsongl thompsongl marked this pull request as ready for review January 23, 2020 19:37
@thompsongl thompsongl requested a review from spalger January 23, 2020 19:47
@thompsongl
Copy link
Contributor Author

thompsongl commented Feb 5, 2020

@chandlerprall @spalger I've added documentation and think this can soon be merged. Having this available in an EUI release will help get the Kibana PR moving

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One comment, otherwise this is great!

wiki/consuming.md Outdated Show resolved Hide resolved
Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>
@thompsongl thompsongl merged commit 034d76d into elastic:master Feb 6, 2020
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.

Build targeting Jest with relevant transforms
2 participants