Stand-alone distributable does not use global PropTypes

[blicksky]

• glamorous version: 4.9.3
• glamor version: 2.20.40
• react version: 15.6.1

Relevant code.

<script src="https://unpkg.com/react@15.6.1/dist/react.js"></script>
<script src="https://unpkg.com/react-dom@15.6.1/dist/react-dom.js"></script>
<script src="https://unpkg.com/prop-types@15.5.10/prop-types.js"></script>
<script src="https://unpkg.com/glamor@2.20.40/umd/index.js"></script>
<script src="https://unpkg.com/glamorous@4.9.3/dist/glamorous.umd.js"></script>

What you did:
Loaded the stand-alone distributions of react, react-dom, prop-types, glamor, and glamorous on a page.

What happened:
The React.PropTypes deprecation warning is printed to the console, even though the prop-types stand-alone distributable file is loaded on the page, and the PropTypes global is available.

react.js:2277 Warning: Accessing PropTypes via the main React package is deprecated, and will be removed in  React v16.0. Use the latest available v15.* prop-types package from npm instead. For info on usage, compatibility, migration and more, see https://fb.me/prop-types-docs
printWarning @ react.js:2277
lowPriorityWarning @ react.js:2296
get @ react.js:275
(anonymous) @ glamorous.umd.js:341
(anonymous) @ glamorous.umd.js:4
(anonymous) @ glamorous.umd.js:5

Reproduction:

This example lists each of the stand-alone distributable files as "External Resources" in the codesandbox configuration on the left-hand side under "Dependencies". There is no meaningful code in any of the other files, as the issue presents itself just by loading this example with the developer console open.

https://codesandbox.io/s/jj4w9m4vx3

Problem description:

Something about the way the glamorous distributable is built with rollup is preventing the prop-types modules from being linked properly, despite it being declared as both a "global" and an "external" in the rollup config. While this only prints a warning now, I believe this will cause failures once React.PropTypes is removed in React 16.

Some more details can be found in the gitter room at https://gitter.im/paypal/glamorous?at=59c51875c101bc4e3af851d6

Suggested solution:

I'm not sure what the right solution is for this.

[kentcdodds]

Hi! Thanks for filing this issue @blicksky!

Hmmm... So the umd build is created using Rollup... So we need some way to tell rollup to include the prop-types require in the UMD build. The config is found here. I'll bet that if we just change this to simply: commonjs() it will "just work"... I'll give that a shot...

[kentcdodds]

Hmmm... I'm just messing around the node_modules version of that file in kcd-scripts and it's not making a difference... If you could dig a little further that'd be great! There's got to be a way to make this work...

[blicksky]

I've been trying a few things, but I haven't had any luck yet. I'm not particularly familiar with rollup, or how kcd-scripts and the rollup config in the glamorous repo play together. One thing that I thought might be related is custom named exports: https://github.com/rollup/rollup-plugin-commonjs#custom-named-exports. I tried updating the rollup config in kcd-scripts to:

    commonjs({
      include: 'node_modules/**',
      namedExports: {
        'node_modules/prop-types/index.js': [ 'PropTypes' ]
      }
    }),

because it appears that the way the prop-types module does exports might be similar to the "hidden" example in the rollup docs, but that didn't seem to work. I may not have been updating kcd-scripts correctly though.

[kentcdodds]

Ok, so I've reproduced things in isolation and filed an issue: rollup/rollup#1646

I'm guessing that I'm just doing something obviously wrong... We'll see.

[blicksky]

rollup/rollup#1385 and rollup/rollup-plugin-commonjs#89 seem to be related. rollup/rollup#1385 is closed as resolved, but it's not clear to me how to take advantage of the claimed support for this in this case.

[blicksky]

I did some experimentation based on what I found in those issues and I was able to get the UMD build working for my use case: master...blicksky:umd-prop-types

While all tests seem to be passing, I don't know if this broke anything for other use cases. If you can help me figure out the best way to validate that this is an appropriate change, I'd be happy to put together a pull request for it.

[kentcdodds]

That wont work because it would require folks install the prop-types and would include it in their bundle, even if they're using an older version of React which supports PropTypes out of the box :-/

[blicksky]

Are you saying that it would just cause the bundle to be unnecessarily large, or would it also not function properly?

[kentcdodds]

It would be unnecessarily large. And I don't want to do this because of Preact support.

However, I just had an idea of something we could do to make this work... Give me some time and I'll make something that works using preval :)

[kentcdodds]

Whoops! That commit was supposed to be in a pull request 😅

Oh well, is fixed now! 🎉

[kentcdodds]

Whoops also, this is the commit that fixed things: ea831b1

[blicksky]

Awesome, thanks so much for the fix and for handling this so quickly!

[kentcdodds]

Sure thing! I used this for this week's newsletter too 😄