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

More robust definition cache for ImportDeclaration #2279

Merged
merged 6 commits into from Sep 5, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Sep 3, 2019

Summary

Fixes #2271, in which some type definition exports remained in the es/ compilation step.

See comment below for full detail and solution.

Checklist

- [ ] Checked in dark mode
- [ ] 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
Copy link
Contributor

chandlerprall commented Sep 4, 2019

This re-introduces the possibility of an infinite dependency loop, for example adding export { COLORS as COLORS2 } from './index'; to button.tsx causes the chain index -> button -> index, throwing RangeError: Maximum call stack size exceeded

The bug - as demonstrated by the files within components/button - is that the first time a file is imported its resulting imports are cached -- however, those results are pruned by the named variables being imported which causes the cache to be invalid.

e.g. given the two imports in two different files:
import { ButtonIconSide } from './button';
import { ButtonColor } from './button';

the script encounters the first file and caches './button' = { ButtonIconSide }, when the second file is processed the same cache is used and ButtonColor is missed.

Two potential ideas for fixing

don't prune exports

Simply commenting out the if (importedTypeNames.includes(name)) { check results in the expected output; this check is in place for two reasons:

  1. code simplicity - without pruning, the file's exports would resolve to all exports found while processing a given file, e.g. given index -> button -> common, index would be seen to have Omit - code would need to updated to prune to the file's exports instead of the values imported from it
  2. performance - don't generate proptypes for unused values; this performance gain recedes when considering the codebase as a whole (most types are exported & used), but there is still overhead for any type and interface defined by a file but not exported.

change cache key

Instead of looking up caches by the resolved file path, the key could instead be the concatenation of the importing file + imported file. This allows pruning to remain as is while resolving the underlying bug.

I think I prefer option 2 as it touches less code and only introduces very marginal complexity.

@thompsongl
Copy link
Contributor Author

Thanks for the deep look @chandlerprall
I went with option 2

@thompsongl thompsongl changed the title Bypass the definition cache for ExportNamedDeclarations More robust definition cache for ImportDeclaration Sep 5, 2019
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.

Changes LGTM; pulled and ran locally against codebase, spot-checked button component files

@chandlerprall
Copy link
Contributor

Thanks again @thompsongl !

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.

still LGTM, but should have a changelog as it affects the content of built files

@thompsongl
Copy link
Contributor Author

@chandlerprall Good call on the CL

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.

❤️

@thompsongl thompsongl merged commit e3b9bb9 into elastic:master Sep 5, 2019
@montogeek
Copy link

Thanks for fixing it :)

thompsongl added a commit to thompsongl/eui that referenced this pull request Sep 10, 2019
* bypass the cache for exportnameddeclarations

* dont set the cache either

* use alternate cache key

* CL
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.

Using EUI 13.x in a standalone project results in import errors
3 participants