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

TypeScript: can't import components individually #1182

Closed
dennari opened this issue Jan 18, 2017 · 13 comments
Closed

TypeScript: can't import components individually #1182

dennari opened this issue Jan 18, 2017 · 13 comments

Comments

@dennari
Copy link
Contributor

dennari commented Jan 18, 2017

When I try to import an individual component, for example

import { Icon } from 'semantic-ui-react/dist/commonjs/elements/Icon';

I get a runtime error:

Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined. You likely forgot to export your component from the file it's defined in. Check the render method of `Component`.

For some reason Icon is undefined. I added a PR which includes a test for this scenario: #1181.

@layershifter
Copy link
Member

I've investigated this issue, problem is quite simple.


When you write this in TS, it's correct because typings doesn't have default export.

import { Icon } from 'semantic-ui-react/dist/commonjs/elements/Icon';
import { IconGroup } from 'semantic-ui-react/dist/commonjs/elements/Icon';

However, when it's transpiled in JS, it's incorrect, Icon/index.js has only default export, so we need to write this:

import Icon from 'semantic-ui-react/dist/commonjs/elements/Icon';
import IconGroup from 'semantic-ui-react/dist/commonjs/elements/Icon/IconGroup';

But, it will not work with TS, because typings says another 🤔 So, we need update typings as:

-export const Icon: IconComponent;
+export default IconComponent;
// ...
-export const IconGroup: React.StatelessComponent<IconGroupProps>;
+const IconGroup: React.StatelessComponent<IconGroupProps>;

But, I'm not sure that this will work after these updates:

import { Icon, IconGroup } from 'semantic-ui-react'

@dennari
Copy link
Contributor Author

dennari commented Jan 21, 2017

Ah now I can see the problem, thanks @layershifter. It seems that the index.d.ts typings files in src/*/* are not actually reflecting the corresponding index.js files. Take the Icon for example:

// src/elements/Icon/Icon.js
...
export default Icon

and

// src/elements/Icon/index.js
export default from './Icon'

but

// src/elements/Icon/index.d.ts
...
export const Icon: IconComponent;
...
export const IconGroup: React.StatelessComponent<IconGroupProps>;

There are two issues here:

  1. index.d.ts doesn't specify a default export but index.js does (the Icon class)
  2. index.d.ts exports IconGroup, but index.js doesn't (it only exports the default export)

There are a couple of ways to solve this:

  1. change the typings to reflect the reality:
// src/elements/Icon/index.d.ts
...
export default IconComponent;
...
// export const IconGroup: React.StatelessComponent<IconGroupProps>;
  1. change the reality to reflect the typings:
// src/elements/Icon/index.js
export { default as Icon} from './Icon'
export { default as IconGroup } from './IconGroup'

I'd vote for option 1., since option 2. isn't backwards compatible.

EDIT: just realized 1. is exactly what @layershifter proposed, sorry for not reading more thoroughly

@dennari
Copy link
Contributor Author

dennari commented Jan 21, 2017

After some testing, in Icon's case for example, the typings would need to be changed as follows:

// src/elements/Icon/index.d.ts
declare const Icon: IconComponent;
export default Icon;

and

// index.d.ts
...
export { default as Icon } from './dist/commonjs/elements/Icon';
...

Note the addition of declare. Also, it won't work if the interface is exported directly.

With this both of these work:

import { Icon } from 'semantic-ui-react';
import Icon from 'semantic-ui-react/dist/commonjs/elements/Icon';

@levithomason
Copy link
Member

@dennari your final solution proposed above #1182 (comment) seems correct to me. In this case, there are separate files for Icon and IconGroup, true?

@levithomason
Copy link
Member

Would you like to add this work to your PR for testing typings, #1181?

@dennari
Copy link
Contributor Author

dennari commented Jan 26, 2017

@levithomason , the most complete solution would involve having a separate typings file for IconGroup as well yes. However I'm not sure if there's a need to export IconGroup separately as it's included as a subcomponent in Icon anyway.

Yes, I'll modify #1181 to reflect this issue once we have agreed exactly what we want to export.

@levithomason
Copy link
Member

it's included as a subcomponent in Icon anyway.

This is true, although, we export all subcomponents at the top level for those who do not want to (or cannot) use the static property notation, see #554.

I'm fine with it either way in this case. Thanks for the update as well.

@layershifter
Copy link
Member

layershifter commented Jan 31, 2017

Here are some thoughts that I have.

Imports

We need to change typings structure to correspond JS-files:

Icon.js
Icon.d.ts
IconGroup.js
IconGroup.d.ts
index.js
index.d.ts

It is reasonable, simple and will solve all problems with imports.

Testing

I think with need to add a new conformance test isTSConformant, it will check that:

  • there is a defintion file for component;
  • a definition file has default export;
  • definition file has an interface ${Component}Props that defines all propTypes.

In last test, we will need to use the AST tree formed by typescript compiler. Test's code can be written in JS, so we can use karma to run it.

@levithomason Other ideas?

@levithomason
Copy link
Member

This is more inline with the direction I was thinking.

The only thought I would add here is that we should consider generating typings completely. It may take quite a bit of work, however, we are investing a large amount of work into constant maintenance as well. I'm beginning to wonder if the work for long term TS maintenance outweighs the work required to create a successful TS interface generator.

@layershifter
Copy link
Member

layershifter commented Feb 5, 2017

@levithomason It's interesting idea, but I think it will be difficult to implement 🔨 However, you and I agree with the changes of typings structure.

I will be glad to see a PR to change the typings structure.

@levithomason
Copy link
Member

Structure updated in the merge of #1395. It would be superb if @dennari could also try that out before shipping it live.

@levithomason
Copy link
Member

#1395 was merged.

@levithomason
Copy link
Member

Released in semantic-ui-react@0.67.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants