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

chore: update react-select to version 3 #819

Merged
merged 4 commits into from
May 31, 2019
Merged

Conversation

montezume
Copy link
Contributor

@montezume montezume commented May 31, 2019

Summary

Updates dependency react-select to version 3.

See this issue for more details on the changes

The main advantage for us is that with version 3, react-select now relies on @emotion 10. This means we won't be shipping two versions of emotion.

I'll update some other deps here too

option => has(option, 'value') && option.value === props.value
) || null,
selectedOptions:
props.isMulti && props.value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to add && props.value because when you clear multi select now, it sends null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to remove this since we will wrap the breaking change

@montezume montezume requested a review from emmenko May 31, 2019 09:06
@montezume montezume self-assigned this May 31, 2019
"treeshaked": {
"rollup": {
"code": 354503,
"import_statements": 910
"code": 429358,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this goes up, but hopefully it's a win due to not shipping two versions of emotion 🤷‍♂ 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, but shouldn't that make the bundle size go down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think size snapshot includes dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, alright, then it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure how size snapshot works to be honest. But I wouldn't hold back a dependency update just to save a few kb 🤷‍♂

@montezume montezume requested a review from adnasa May 31, 2019 09:10
@montezume
Copy link
Contributor Author

I'll do the react-testing-library update in another pr

@montezume montezume requested a review from jonnybel May 31, 2019 09:35
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

LGTM 👌

AsyncCreatable as AsyncCreatableSelect,
} from 'react-select';
import { components as defaultComponents } from 'react-select';
import AsyncCreatableSelect from 'react-select/async-creatable';
Copy link
Member

Choose a reason for hiding this comment

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

Are they doing this on purpose to have better treeshaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

this is what they said

"This should have no bundle-size impact on react-select consumers currently leveraging tree-shaking. However for consumers who aren’t leveraging tree-shaking, this should help alleviate some of the bundle-weight."

@montezume montezume merged commit 7f294ac into master May 31, 2019
@montezume montezume deleted the ml-use-react-select-3 branch May 31, 2019 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants