Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

feat(preact): add native support for preact #321

Merged
merged 2 commits into from
Sep 12, 2017

Conversation

marzelin
Copy link
Collaborator

@marzelin marzelin commented Sep 3, 2017

What: Adds out-of-the-box support for preact

Why: Even though glamorous can be used in preact project, there are a few issues:

  • there are some component props that are allowed in preact but invalid in react.
  • React is a peer-dependency of glamorous, which means you get a warning if you don't have it installed.
  • preact-compat is required to be able to use glamorous

How:
Build process creates preact directory that stores files dedicated for this library. To use them one needs to import glamorous from glamorous/preact.

preact files differ from normal files in a following way:

  • preact-specific props are added to props list in ./react-props
  • PropTypes are neutralized (preact doesn't use them) and Children polyfilled in ./react-compat.
  • All react imports are replaced with preact (at build time).

Other changes:
react is removed from peerDependencies in package.json
A shim of package.json is added to preact dir (at build time) to allow for importing files from there.
typings folder is copied to preact folder to provide type definitions for typescript

closes #85

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is so good! Thank you so much for this work! I'll want to do a pre-release of this before merging just to make sure we're good to go. I'll let you know when I've done that. Thanks again!

@@ -0,0 +1,3 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add this file to the gitignore? I don't like editor-specific settings in my repositories 😀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I need this file because my prettier rules are different and they're applied on each save. I'll keep it only in my fork.

@@ -0,0 +1,19 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

package.json file needs to be present in preact folder since node.js looks into it to figure out which file to import. This file is used as a blueprint for it. Perhaps I could use main package.json file for this as well 🤔

@luke-john
Copy link
Collaborator

typings folder is copied to preact folder to provide type definitions for typescript

I'm not familiar with preact or it's typings, but have you tested that this works, seems like there's a reasonable chance you'll get type failures with the react typings conflicting with preact ones.

If this is a problem I think #320 should improve the situation.

I'm unlikely to be able to spend much time looking into how the typings play with preact this week, but love the idea that the typings can be shared ❤️. Is there something like create-react-app for preact I can give this a quick spin?

@kentcdodds
Copy link
Contributor

I've published this as the next beta!

~/Desktop/glamorous (master)
👻  $ npm publish --tag preact
+ glamorous@4.8.0-beta.0

Please try it out and let me know whether it works.

@marzelin
Copy link
Collaborator Author

marzelin commented Sep 4, 2017

@luke-john there's preact-cli but it contains a lot of packages, including preact-compat. I've created minimal typescript+preact+glamorous project and you're right, typescript throws errors since react is also imported in type definitions, unfortunately.

Apart from that everything seems to work. Here's a playground that uses beta umd build.

@luke-john
Copy link
Collaborator

Thanks for that @marzelin.

I think a nice way to solve this could be to have a file which re exports aliases of the react interfaces used by the typings. And then as part of the copy step for preact replace that file with one that has the preact equivalents.

@codecov-io
Copy link

codecov-io commented Sep 6, 2017

Codecov Report

Merging #321 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #321   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         172    174    +2     
  Branches       47     48    +1     
=====================================
+ Hits          172    174    +2
Impacted Files Coverage Δ
src/with-theme.js 100% <ø> (ø) ⬆️
src/index.js 100% <ø> (ø) ⬆️
src/get-glamor-classname.js 100% <100%> (ø) ⬆️
src/theme-provider.js 100% <100%> (ø) ⬆️
src/react-props.js 100% <100%> (ø)
src/create-glamorous.js 100% <100%> (ø) ⬆️

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 b41be21...e725ee1. Read the comment docs.

Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! Just another question :)

beforeEach(() => {
process.env.PREACT = true
jest.resetModules()
jest.mock('react', () => ({}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? Should it be: jest.mock('react', () => require('preact')) instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I've added preact as a dev dependency after all. Good idea 👏

'build.umd.main.preact.tiny',
'build.umd.min.preact.tiny'
),
copy("'package.json' 'preact'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... What does this do exactly?

I'm a little confused about the build process. When everything's finished, what are we left with?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It creates a directory preact in which the are all builds for preact as well as a copy of package.json from the root directory. No typings at this time since current type definitions are react-dependent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! So, someone using the package would do:

import glamorous from 'glamorous/preact'

Is that right? That makes good sense. For downshift I push everything into the dist/ and have .preact filename, but I think I like this better. Thanks!

Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

I think that I want 2 more things before I merge this:

  1. A PR on the website to document how to use preact (bonus points if it includes a reference to using babel-plugin-module-alias to alias glamorous to glamorous/preact and/or the webpack method)
  2. One more beta release and some users verifying that it works properly.

This is so awesome. Thank you so much for putting all the work into this!

'build.umd.main.preact.tiny',
'build.umd.min.preact.tiny'
),
copy("'package.json' 'preact'")
Copy link
Contributor

Choose a reason for hiding this comment

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

I see! So, someone using the package would do:

import glamorous from 'glamorous/preact'

Is that right? That makes good sense. For downshift I push everything into the dist/ and have .preact filename, but I think I like this better. Thanks!

@kentcdodds
Copy link
Contributor

I'm working on this right now. I want to add kcd-utils to this project, so I'm just going to stick that in as part of this PR :)

@kentcdodds kentcdodds changed the title [WIP] feat(preact): add native support for preact feat(preact): add native support for preact Sep 12, 2017
@kentcdodds
Copy link
Contributor

I literally spent all day on this 😂

But I think I've finally got it working a great way.

Thank you for all your work on it @marzelin!

@kentcdodds
Copy link
Contributor

Ok! I'll merge this and the docs PR as soon as I get one person to confirm that this works with preact-cli:

~/Desktop/glamorous (master)
🎉  $ npm publish --tag beta
+ glamorous@4.9.0-beta.0

@kentcdodds
Copy link
Contributor

Not quite there. I just tested it and something's messed up. Should be a quick fix...

@kentcdodds
Copy link
Contributor

Awesome! I've verified that glamorous@4.9.0-beta.1 (this PR) works great with preact-cli. preact-cli will pick up the preact/dist/glamorous.esm.js file when you do:

import glamorous from 'glamorous/preact'

🎉

@kentcdodds kentcdodds merged commit 2a57031 into paypal:master Sep 12, 2017
@kentcdodds
Copy link
Contributor

Tweeted. Sorry, I couldn't find your twitter handle :-/

Thanks so much!!

@kentcdodds
Copy link
Contributor

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the CONTRIBUTING.md and other/MAINTAINING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@marzelin
Copy link
Collaborator Author

Awesome 🎉. Thanks @kentcdodds

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.

Make glamorous work out of the box with preact
4 participants