Skip to content

Dont repeat full object names where possible #4083

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

Closed
wants to merge 1 commit into from

Conversation

mbrevda
Copy link

@mbrevda mbrevda commented Oct 21, 2016

A tiny size gainz for PropTypes


export const action = PropTypes.oneOf([
const FuncReq = func.isRequired
const StrReq = StrReq
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this read const StrReq = string.isRequired?

Copy link
Author

Choose a reason for hiding this comment

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

search & replace. Fixed now.

@@ -1,48 +1,50 @@
import { PropTypes } from 'react'
const {func, string, any, oneOf, shape} = PropTypes
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add spaces inside of the brackets. That's consistent with how we normally write destructurings.

Copy link
Member

Choose a reason for hiding this comment

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

if we're gonna nit on this stuff I suppose we should put it in the eslint config.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's come up a couple times before. I can get that added.

@ryanflorence
Copy link
Member

just curious, what's the size change?

@mbrevda
Copy link
Author

mbrevda commented Oct 22, 2016

@ryanflorence embarrassingly small (like .5k or so). This was more as a sodlace when I realized that #4046 wasn't going to happen

@alisd23
Copy link
Contributor

alisd23 commented Oct 22, 2016

Is the .5k really worth it? Seems like it's complicating the PropTypes file for a really tiny gain.

@alisd23
Copy link
Contributor

alisd23 commented Oct 22, 2016

I can understand destructing the PropTypes variable, but is the func.isRequired -> funqReq really necessary? After minification aren't we looking at a difference of 10's of characters?

@mbrevda
Copy link
Author

mbrevda commented Oct 23, 2016

what's the size change?

I've confirmed that my bundle size decreases by .5k

Is the .5k really worth it?

I think that's subjective. Personally, I'd love to drop every single byte that I can.

is the func.isRequired -> funqReq really necessary

I appreciate not minifying code at the expense of readability. I think that propType-like variables are pretty obvious to most. I wouldn't necessarily rename methods or classnames on the basis of minification, and the exported values are quite self documenting.

I do agree that a babel-transform to do this work at build time would be preferable. I do not know of any that can do that for us though.

I should point out that most libs wouldn't have to deal with this issue as they can strip out propTypes at build time (which was the original motivation here). However libraries that use context may wish to take this extra step.

@ryanflorence
Copy link
Member

I'd rather not worry about micro stuff like this right now, trying to ship this and get it right first.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants