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

Implements nesting in template string parser #1103

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

kof
Copy link
Member

@kof kof commented May 12, 2019

Implements #837

I think I found a way to implement it without degrading performance, LOC is still pretty small.

The problem it is solving: you can't express a nested rule with a template string. If you used a template string initially to describe the rule, as soon as you want to add nesting, you will have to rewrite it to objects.

const styles = {
  btn: `
    float:  left;
    & span: {
      color: red;
    }
  `
}

@kof kof requested a review from HenriBeck as a code owner May 12, 2019 23:06
@kof kof requested a review from oliviertassinari May 12, 2019 23:07
@kof
Copy link
Member Author

kof commented May 12, 2019

Also cc @eps1lon

@@ -44,7 +44,7 @@ module.exports = config => {
frameworks: ['benchmark'],
// Using a fixed position for a file name, m.b. should use an args parser later.
files: [process.argv[4] || 'packages/jss/benchmark/**/*.js'],
preprocessors: {'packages/jss/benchmark/**/*.js': ['webpack']},
preprocessors: {'packages/**/benchmark/**/*.js': ['webpack']},
Copy link
Member Author

Choose a reason for hiding this comment

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

was broken

@@ -4,7 +4,7 @@ import template from '../../src/index'
import parse from '../../src/parse'

const options = {Renderer: null}
const jss = create(options).use(template())
const jss = create(options).use(template({cache: false}))
Copy link
Member Author

Choose a reason for hiding this comment

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

disabling cache here to have a realistic comparison without caching, I didn't document it, because I don't think user should need it, so for now it's a private one, just for tests

package.json Outdated
@@ -15,7 +15,7 @@
"format:ci": "yarn format --list-different",
"test": "karma start --single-run",
"test:watch": "karma start",
"test:bench": "cross-env BENCHMARK=true karma start --single-run",
"test:bench": "cross-env NODE_ENV=production BENCHMARK=true karma start --single-run",
Copy link
Member Author

Choose a reason for hiding this comment

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

removes dead from the bench~!

suite('Parse', () => {
benchmark('parse()', () => {
parse(css)
})

benchmark('stylis()', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

using stylis parser just to see a comparable other value in perspective

Copy link
Contributor

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Exciting stuff.

packages/jss-plugin-template/src/index.js Show resolved Hide resolved
if (openCurlyIndex === -1) {
warning(false, `[JSS] Missing opening curly brace in "${decl}".`)
}
const key = decl.substring(0, openCurlyIndex - 1)
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 trim the selector here?

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

}
const key = decl.substring(0, openCurlyIndex - 1)
const nestedStyle = {}
rules[rules.length - 1][key] = nestedStyle
Copy link
Member

Choose a reason for hiding this comment

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

Why do we add the key here to the last object?

Copy link
Member Author

Choose a reason for hiding this comment

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

rules array represents an array of nested styles, each level, separate array entry, so we need to do both, reference the nested style in the parent object and add it to the array to have a pointer to the current style object where declarations will be added next in that pass


// We are closing a nested rule.
if (decl === '}') {
rules.pop()
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 we add the rule when we pop it to the style?

Copy link
Member Author

Choose a reason for hiding this comment

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

we already did, we are just removing it from the array, since we basically finished adding declarations to that rule

@HenriBeck
Copy link
Member

@kof should we merge this?

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