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

feat: support more element types #617

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

Andarist
Copy link
Contributor

No description provided.

@Andarist Andarist changed the title Support more element types feat: support more element types Sep 27, 2021
Copy link
Collaborator

@armandabric armandabric left a comment

Choose a reason for hiding this comment

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

Wow! Thanks for this really god PR. I just have a question regarding the rollup configuration part.

Comment on lines -39 to -44
commonjs({
sourceMap: true,
namedExports: {
react: ['isValidElement'],
},
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why you have to change that in the context of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commonjs plugin is only needed when you actually want to bundle a commonjs dependency - you don't really want that with react since you are leaving it as external (like you should), so this was actually a "dead"/redundant config

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's improve this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, could you be more specific? maybe I've explained this poorly - so let me rephrase: the removed code that was not actually used and it wasn't doing anything here. And how this config now is defined is correct - from my point of view this change is "sound" and correct. There is no regression here so this can be merged "as is". Please let me know if this addresses your concerns - if not I can try to explain it further and in more detail.

Copy link
Collaborator

@armandabric armandabric Sep 28, 2021

Choose a reason for hiding this comment

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

Ho! Sorry I just wanted to said Let's change this! in the meaning of accepting your change with pleasure. Sorry for the wrong wording 🙊

@Andarist
Copy link
Contributor Author

@armandabric I will look into the Flow issue reported by CI soon-ish, should be a trivial fix

@armandabric
Copy link
Collaborator

armandabric commented Sep 28, 2021

This PR will fix #352 and is related to #617 #613

@Andarist
Copy link
Contributor Author

@armandabric I've fixed the Flow issue 🎉 Also - I think that you have wanted to link another issue/PR since #617 is this one

Comment on lines 74 to 75
// this is "unknown" case and will most likely result in `[object Object]`
// it's fine though as we can't display anything meaningful anyway here
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you prefer returning "UnknownElementType" here or something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like your idea to return UnknownElementType. It will help to debug issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@armandabric armandabric left a comment

Choose a reason for hiding this comment

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

Thanks again for this PR! That was a long time running issue for the library 👌

@xyy94813
Copy link

For @storybook/addon-docs, the scenario of converting a lazy component to a string is a little different from the needs of react-dev-tools.

function MyComponent() {
  return 
}

const LazyComponent = React.lazy(async () => {
  return MyComponent
});

const root = React.render(
  <LazyComponent />
);

// for react-dev-tools
expect(jsxToString(root)).toBe(<Lazy />);  // first
expect(jsxToString(root)).toBe(<MyComponent />); // then

// for `@storybook/addon-docs` i will except
expect(jsxToString(root)).toBe(<Lazy(MyComponent) />); // then

Any suggestions about element to Source code?

@Andarist
Copy link
Contributor Author

Ah yes - I was wondering about this case but the problem here is that this LazyComponent doesn't have a name at all. We could try to infer it from the inner function but that is not an established pattern for React.lazy. We can't easily access the MyComponent name in the given example during the runtime.

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.

3 participants