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

Configurable imports #290

Merged
merged 10 commits into from
Sep 12, 2017
Merged

Conversation

ChristopherBiscardi
Copy link
Member

@ChristopherBiscardi ChristopherBiscardi commented Sep 1, 2017

Built on top of lint-fix changes (#289) -- compare between these two branches

What:

Solves #267 by implementing a solution that will use the default import name instead of sticking only to styled.

in short, this works:

import lol from 'emotion';

lol`color: blue`;

Why: See #267

How:

Modified the visitor to also visit ImportDeclarations. Then cache the values of any import whatever from 'emotion' in an object for later use when looking for styled calls.

Checklist:

These need work. This is just a POC. There definitely needs to be more tests.

  • Documentation
  • Tests
  • Code complete

return {
name: 'emotion', // not required
inherits: require('babel-plugin-syntax-jsx'),
visitor: {
ImportDeclaration: {
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this below Program?

export default function(babel) {
const { types: t } = babel

let importedNames = {
Copy link
Member

Choose a reason for hiding this comment

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

The importedNames arg values come from state.opts. We might also want to set the imported names to state.importedNames

return {
name: 'emotion', // not required
inherits: require('babel-plugin-syntax-jsx'),
visitor: {
ImportDeclaration: {
enter({ node }) {
if (node.source.value === 'emotion') {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be node.source.value.indexOf('emotion') !== -1 so it includes emotion, react-emotion, preact-emotion and any other runtimes people create.

@codecov-io
Copy link

codecov-io commented Sep 3, 2017

Codecov Report

Merging #290 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   90.37%   90.45%   +0.07%     
==========================================
  Files          21       21              
  Lines         966      974       +8     
  Branches      260      261       +1     
==========================================
+ Hits          873      881       +8     
  Misses         75       75              
  Partials       18       18
Impacted Files Coverage Δ
packages/babel-plugin-emotion/src/index.js 97.87% <100%> (+0.12%) ⬆️
packages/babel-plugin-emotion/src/css-prop.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 e7b36da...9c9df43. Read the comment docs.

@tkh44 tkh44 requested review from tkh44 and emmatown September 6, 2017 03:19
@tkh44 tkh44 added the merge me! label Sep 6, 2017
}

const defaultImportedNames = {
default: 'styled',
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this from default to styled

@emmatown emmatown changed the title POC Configurable imports Configurable imports Sep 6, 2017
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

I'm good with this as long as it's explicitly stated in the docs that it only works for ESM imports and all imports are supported

// Is default import
names.default = singleImport.local.name
}
})
Copy link
Member

Choose a reason for hiding this comment

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

This can be done with state.file.metadata.modules.imports in the Program enter visitor.

http://astexplorer.net/#/gist/39f0d575f432d2a93f29c0aebe34956c/36992fb4d485246b651b79d3579756ef016c9c5c

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Was able to remove parseImports and the ImportDeclaration visitor.

@@ -0,0 +1,101 @@
# Configurable Imports

If you are using ES Module imports (`import styled from 'emotion'`)
Copy link
Member

Choose a reason for hiding this comment

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

styled doesn't exist in emotion. It's only in react-emotion and preact-emotion.


```js
import _JSXStyle from "styled-jsx/style";
import styled, { css } from "react-emotion";
Copy link
Member

Choose a reason for hiding this comment

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

should this say css as cows?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Basically this is a POC that proves that this works without extra
configuration. Breaks no existing tests. Only works for `styled`
calls.

```js
import lol from 'emotion';

lol`color:blue`;
```
Set importedNames on state and merge with babel opts.
* Remove ImportDeclaration visitor
* Remove parseImports
* Move import naming logic to `Program`
@tkh44 tkh44 dismissed emmatown’s stale review September 12, 2017 16:24

The changes were addressed

@tkh44 tkh44 merged commit 38a58f9 into emotion-js:master Sep 12, 2017
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.

4 participants