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

fix: allow prop-types to be removed in production #258

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

nolanlawson
Copy link

@nolanlawson nolanlawson commented Dec 18, 2018

fixes #257

There were a few things required in order to actually remove prop-types from the build:

  1. Split shared-props.js into shared-props.js and shared-default-props.js, because one needs to be removed and the other one doesn't.
  2. Use the additionalLibraries option in babel-plugin-transform-react-remove-prop-types to catch shared-props.js in addition to prop-types
  3. Update the babel transform.
  4. Use removeImport: true.
  5. Add comment annotations to help give the babel transform some hints about what to remove.

I can confirm that this fixes the issue by running:

egrep -r '(prop-types|shared-props)' dist dist-es

And I observe that prop-types is only imported by shared-props.js, and shared-props is not referenced by any other module.

Now, this PR raises the question of whether we actually want to unilaterally remove the prop-types from the npm package. If we do, then we lose the debugging benefits.

However, I tried using the "mode": "wrap" option of the babel transform (to wrap everything in a process.env.NODE_ENV === "production" check, and it ran into a runtime error for some reason.

So perhaps a better solution is to build multiple versions:

dist/
dist-es/
dist-prod/
dist-prod-es/

Alternatively, we could make these code changes but not actually run the transform ourselves, but then provide instructions in the readme for configuring the transform. Then consumers of this library would be able to decide for themselves if they want to remove the prop-types.

@nolanlawson
Copy link
Author

On second thought, I would really like to solve this in such a way that we still retain the prop-types for developers. Otherwise there is little point in having them at all.

@nolanlawson
Copy link
Author

Hm, I can't seem to get babel-plugin-transform-react-remove-proptypes to work properly in "wrap" mode, even when downgrading due to oliviertassinari/babel-plugin-transform-react-remove-prop-types#172. (When downgraded, it just prepends a false ? check which is not what we want.)

Also even if I could get it to work, potentially it would add its own issues since we'd have introduced process globals into our dist code where it didn't exist before.

we could make these code changes but not actually run the transform ourselves, but then provide instructions in the readme for configuring the transform.

^ This seems like the most sensible and least invasive option.

@nolanlawson
Copy link
Author

nolanlawson commented Mar 8, 2019

OK, now this PR is just a refactoring to make prop-types removable, plus some instructions in the README for how to remove prop-types. Also I removed babel-plugin-transform-react-remove-prop-types from our own dependencies, as it wasn't doing anything anyway.

@nolanlawson nolanlawson changed the title fix: actually remove prop-types from build fix: allow prop-types to be removed in production Mar 8, 2019
@nolanlawson
Copy link
Author

This seems pretty uncontroversial to me now, so I'm going to self-approve and merge

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.

prop-types are not actually removed in production
1 participant