feat: allow importing built-in glamorous components #131

marzelin · 2017-05-24T19:12:13Z

What:
allow something like import { Div, Span } from 'glamorous'

Why:
Convenient helpers improve user experience.

How:
Create a build script that appends exports to the es file.

Checklist:

Closes #123

Sidenote:
This is not related directly to this commit, but my liter shows an error in dist/glamorous.es.js:
'A module cannot have multiple default exports.'
That's caused by line 1177: exports.default = reactHtmlAttributes$2;

kentcdodds · 2017-05-24T19:15:54Z

Hi! Thanks so much for this! I'm going to pull it down and run it to double check, but it looks pretty great.

Here are the linting failures. You may want to run npm start validate to check things before you push your changes :)

kentcdodds · 2017-05-24T19:16:48Z

I would love it if we could also get a test in the dist-tests to check whether you can import things as we want to. Can do it with just one or two elements I think. Also documentation updates would be useful.

kentcdodds · 2017-05-24T19:17:34Z

As for your linter issues, I think your linter is busted unless I'm missing something. That file is fine I think.

kentcdodds · 2017-05-24T19:20:21Z

other/concat-exports.js

+  .map(tag => {
+    const validJsName = dashToCamelCase(tag)
+    const capitalName = capitalize(validJsName)
+    return `export const ${capitalName} = glamorous['${tag}']`


Let's add a semicolon here because this is our transpiled code, we should never rely on ASI.

codecov-io · 2017-05-24T22:15:21Z

Codecov Report

Merging #131 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #131   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         130    130           
  Branches       33     33           
=====================================
  Hits          130    130

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b742428...57c8a4a. Read the comment docs.

marzelin · 2017-05-24T22:17:57Z

fix bug where component names collide with JavaScript constructors
tests added
documentation added

kentcdodds

Looking good! Just a few comments :)

kentcdodds · 2017-05-25T02:06:51Z

other/concat-exports.js

+  return s.replace(/-([a-z])/g, ([, char]) => char.toUpperCase())
+}
+
+const jsBuiltInObjectNames = [


I wonder whether there's an npm package (probably by Sindre Sorhus) that has these so we don't have to hard-code them 🤔

Found one :) https://github.com/jonschlinkert/reserved

the reserved package wasn't updated in 3 years and is a bit outdated. I can use node.js to generate the list.

kentcdodds · 2017-05-25T02:07:26Z

other/concat-exports.js

+]
+
+function unCollide(name) {
+  const prefix = 'html'


Very interesting! Makes total sense. Could you explain why (perhaps in a code comment) the prefix html is used? Seems like a good one, just curious to know why.

Oh, also, this should probably be capitalized. All the others have the first letter capitalized so it's:

import {HtmlObject, Article} from 'glamorous'

Rather than:

import {htmlObject, Article} from 'glamorous'

I chose html because users may already be familiar with this prefix since it is used in JavaScript (htmlFor). But since component names have to be capitalized (otherwise React/JSX treats the tag as custom element), I think postfix Tag (ie ObjectTag) is a nicer alternative. What do you think?

souporserious · 2017-05-25T06:29:39Z

other/concat-exports.js

@@ -0,0 +1,88 @@
+const appendToFile = require('fs').appendFile


Are you getting a warning with this? I had to switch to appendFileSync to get rid of it.

I'm getting no warnings. I think async execution is not super useful here, so I can switch to sync.

marzelin · 2017-05-25T07:59:24Z

change html prefix to Tag postfix when name collisions
automatically create a constructor names list
change appendFile to appendFileSync

kentcdodds

This is looking so great. Thank you again for working on it. Just a few comments. Also, I wonder if there's a way for us to verify that the tree shaking works 🤔 Maybe we could add an example to the /examples? We could probably do that in a separate PR. @souporserious, would you be willing to work on that? It should be something really simple. Could even be with CRA and we commit the bundle.js where you can see that it's been tree-shaken.

kentcdodds · 2017-05-25T12:59:25Z

README.md

@@ -131,6 +131,8 @@ If you're transpiling (and/or using the `jsnext:main`):

 ```javascript
 import glamorous, {ThemeProvider} from 'glamorous'
+// you can also import specific components
+import {Div, H2} from 'glamorous'


Could you include a line about the special case for JavaScript built-ins like Object? Something like:

// Tags with the same name as built-in JavaScript objects are importable with a Tag suffix, IE: ObjectTag.

kentcdodds · 2017-05-25T13:02:01Z

other/concat-exports.js

+    return `export const ${tagName} = glamorous['${tag}'];`
+  }).join`\n`
+
+const toWrite = `\n${elementExports}`


It would be handy to add a comment for the first line that says:

// these exports below are generated and will be tree-shaken if you're using Webpack 2 or Rollup

marzelin · 2017-05-25T16:12:16Z

add a comment preceding exports
provide info about Tag suffix in documentation

kentcdodds

Reeeally close! Thanks for your tireless iteration on this 👍

kentcdodds · 2017-05-25T16:16:57Z

other/concat-exports.js

+}
+
+// node version <= 7 does not support trailing commas in function calls
+// prettier-ignore


kentcdodds · 2017-05-25T16:17:47Z

other/concat-exports.js

+  return !!s && s[0] === s[0].toUpperCase()
+}
+
+// node version <= 7 does not support trailing commas in function calls


Normally, I'll add this at the end of files which are not transpiled:

// this is not transpiled /* eslint max-len: 0, comma-dangle: [ 2, { arrays: 'always-multiline', objects: 'always-multiline', functions: 'never' } ] */

kentcdodds · 2017-05-25T16:18:04Z

other/concat-exports.js

+// node version <= 7 does not support trailing commas in function calls
+// prettier-ignore
+const jsBuiltInConstructorNames = Object.getOwnPropertyNames(global)
+  .filter(isCapitalized)


I'm unsure whether this is really necessary 🤔

It's not necessary but reduces the number of items to compare from 63 to 40 which improves performance a bit.

I'm more concerned about readability/maintainability of this file than performance and would prefer to not include anything in here that's only here for performance reasons. The script is really fast as it is :)

Well, I think this code is quite readable but of course less code is always a plus. I'll remove it.

Yeah, it's not about how much code, it's about the fact that you need to decide why this matters, and the fact is that it doesn't ;-)

Thanks so much!

We're discussing it so it kind of matters (or it's just bikeshedding) ;)

kentcdodds · 2017-05-25T16:19:37Z

other/concat-exports.js

+    const capitalName = capitalize(validJsName)
+    // add postfix if name collides with js constructors
+    const tagName = unCollide(capitalName)
+    return `export const ${tagName} = glamorous['${tag}'];`


Ah! Close call, this should actually be:

return `export const ${tagName} = glamorous['${capitalName}'];`

I was wondering what to put there. Isn't glamorous.div equal to glamourous.Div?

It is not. Here's some terminology that should hopefully be helpful:

glamorous is a "componentFactoryFactory"

glamorous.div is a "componentFactory" which is created by calling glamorous('div')

glamorous.Div is a "component" which is created by calling (effectively) glamorous('div')() (in addition to a few extra things)

We don't (and wont?) support exporting the component factories, but exporting the components themselves could be useful.

I hope that helps clarify :)

I should've RTFM 😊 sorry

Why don't we just remove componentFactory and move its competencies onto a glamorous component? That would make things simpler.

I've considered this, and I'm not totally against it. I think that it could make things even more complex from both an API and implementation standpoint, but I'm open to talk about it in another issue if you'd like to bring it up.

kentcdodds · 2017-05-25T16:21:24Z

README.md

@@ -131,6 +131,12 @@ If you're transpiling (and/or using the `jsnext:main`):

 ```javascript
 import glamorous, {ThemeProvider} from 'glamorous'
+
+// you can also import specific components


We should probably call out the fact that these are the built-in components by saying:

Please read the section below on "Built-in" components.

marzelin · 2017-05-25T20:28:39Z

export GlamorousComponents instead of GlamorousComponentFactories
remove unnecessary function
improve documentation
make tests more robust

kentcdodds · 2017-05-25T20:30:36Z

README.md

+
+// tags with the same name as built-in JavaScript objects are importable with a Tag suffix
+// and tag names that contain dashes are transformed to CamelCase
+import {MapTag, ColorProfile} from 'glamorous'


This is perfect!

kentcdodds · 2017-05-25T20:31:46Z

Wonderful! Thank you!

kentcdodds reviewed May 24, 2017

View reviewed changes

atticoos mentioned this pull request May 25, 2017

feat(glamorous): export builtin components robinpowered/glamorous-native#41

Merged

kentcdodds reviewed May 25, 2017

View reviewed changes

souporserious reviewed May 25, 2017

View reviewed changes

kentcdodds reviewed May 25, 2017

View reviewed changes

feat: allow importing built-in glamorous components

57c8a4a

kentcdodds approved these changes May 25, 2017

View reviewed changes

kentcdodds merged commit 789b97e into paypal:master May 25, 2017

kentcdodds mentioned this pull request Jun 27, 2017

Add flow type definitions #177

Closed

		@@ -0,0 +1,88 @@
		const appendToFile = require('fs').appendFile

feat: allow importing built-in glamorous components #131

feat: allow importing built-in glamorous components #131

Conversation

marzelin commented May 24, 2017 • edited Loading

kentcdodds commented May 24, 2017 • edited Loading

kentcdodds commented May 24, 2017

kentcdodds commented May 24, 2017

Choose a reason for hiding this comment

codecov-io commented May 24, 2017 • edited Loading

Codecov Report

marzelin commented May 24, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marzelin commented May 25, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marzelin commented May 25, 2017

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marzelin commented May 25, 2017

Choose a reason for hiding this comment

kentcdodds commented May 25, 2017

marzelin commented May 24, 2017 •

edited

Loading

kentcdodds commented May 24, 2017 •

edited

Loading

codecov-io commented May 24, 2017 •

edited

Loading