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

Remove TS types from exports when they come from node_modules #2191

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Aug 1, 2019

Summary

Fixes #2185 by filtering out non-value exports from node_modules during build/babel. I tested by:

  • diffing the es directory before-and-after this change, only components/drag_and_drop/index.js had changes
  • created a new CRA repo and included and built with an npm packed version of EUI with these changes

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

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.

Built locally and output in es/ drag and drop still has export {} from 'react-beautiful-dnd';, but that seems harmless.

Local CRA-EUI project doesn't throw errors.

// comes from a 3rd-party library
// best way to reliably check if this is
// a type or value is to require the
// library and check it's exports
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I didn't notice any increase in build time, but I'd expect this method to take longer, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only executes for export {} from '' statements, which we don't really do with node_modules libraries.

@chandlerprall
Copy link
Contributor Author

Built locally and output in es/ drag and drop still has export {} from 'react-beautiful-dnd';, but that seems harmless.

We've had export {} from './color_types'; in es/services/color/index.js for a while with no issue. I took a pass at removing the empty exports, but ended up causing some unintended consequences. We can take another look in the future if we want to.

@chandlerprall chandlerprall merged commit 128797b into elastic:master Aug 2, 2019
@chandlerprall chandlerprall deleted the bug/2185-remove-3rd-party-type-exports branch August 2, 2019 15:21
thompsongl pushed a commit to thompsongl/eui that referenced this pull request Sep 10, 2019
…c#2191)

* Remove TS types from exports when they come from node_modules

* changelog

* Remove debugger statement
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.

Types exported directly from react-beautiful-dnd are not removed in ES build
2 participants