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

[WIP] Babel plugin for preprocessing #757

Closed
wants to merge 49 commits into from
Closed

[WIP] Babel plugin for preprocessing #757

wants to merge 49 commits into from

Conversation

kof
Copy link
Member

@kof kof commented Jul 14, 2018

Issue #579

kof added 5 commits July 13, 2018 16:33
- custom identifiers
- rule ref
- null/undefined as styles
- property identifier
- numeric values
@kof kof self-assigned this Jul 14, 2018
const defaultIdentifiers = ['createStyleSheet', 'injectSheet']

export default declare(({types: t, ...api}, {identifiers = defaultIdentifiers, jssOptions}) => {
api.assertVersion(7)
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe update our own babel packages to v7 as well? Would make testing easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, if you get time, v6 and 7 are largely compatible

@kof
Copy link
Member Author

kof commented Jul 20, 2018

@cssinjs/core anyone wants to review this one? I am in the testing phase yet, but I want already start with making this code high quality readable

@HenriBeck
Copy link
Member

Can you rename the directory from babel-plugin to babel-plugin-jss?

@@ -0,0 +1,14 @@
import * as babel7 from '@babel/core'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename to just babel?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working on tests to ensure babel6 works, so it will be needed

@HenriBeck
Copy link
Member

Also, can we have one standard for where the test files should be located? Because right now sometimes *.test.js is used and sometimes all of the tests are inside a tests folder.

@kof
Copy link
Member Author

kof commented Jul 21, 2018

Can you rename the directory from babel-plugin to babel-plugin-jss?

yep

@kof
Copy link
Member Author

kof commented Jul 21, 2018

Also, can we have one standard for where the test files should be located? Because right now sometimes *.test.js is used and sometimes all of the tests are inside a tests folder.

There are 2 different testing scenarios. One is unit tests, where I actually test a specific module. The other one is more like an integration tests. In this case I 100% did integration tests and I had them all in index.test.js but then the amount of tests grew and file became unreadable, so I split them up in separate suits.

I think its ok to not have this consistency. If you are testing a specific module, then it should be module.test.js, otherwise if a test uses multiple modules it should go into __tests__. Both patterns are supported by jest by default without changing the config.

@kof
Copy link
Member Author

kof commented Jul 27, 2018

Remaining todo:

  • find a way to identify function calls without relying on a name, because this doesn't work after certain babel plugins, it be something.default()
  • find a way to extract imported static variables
  • evaluate module code in a separate context instead of eval
  • add tests ensuring the proper variable scoping when evaluating (use same variable name to detect wrong scoping)
  • use evaluation more instead of nodes traversing, seems like a more robust appraoch
  • test on production code bases

"@babel/helper-plugin-utils": "^7.0.0-beta.53",
"babel-generator": "^6.26.1",
"babel-types": "^6.26.0",
"jss": "^9.8.7",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't jss be a peer dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

seems valid

@jahglow
Copy link

jahglow commented Mar 22, 2019

guys, any progress on this? Like css extraction I mean...

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.

3 participants