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

Incorporate styled-jsx #420

Merged
merged 9 commits into from
Dec 19, 2016
Merged

Incorporate styled-jsx #420

merged 9 commits into from
Dec 19, 2016

Conversation

nkzawa
Copy link
Contributor

@nkzawa nkzawa commented Dec 17, 2016

This PR adds styled-jsx support.
It's almost ready but need to wait the fix of vercel/styled-jsx#41

@coveralls
Copy link

Coverage Status

Coverage increased (+2.7%) to 59.834% when pulling a73047e on add/styled-jsx into b62a0e8 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+2.7%) to 59.834% when pulling a73047e on add/styled-jsx into b62a0e8 on master.

@nkzawa nkzawa changed the title [WIP] Incorporate styled-jsx Incorporate styled-jsx Dec 19, 2016
@nkzawa
Copy link
Contributor Author

nkzawa commented Dec 19, 2016

Done

@nkzawa
Copy link
Contributor Author

nkzawa commented Dec 19, 2016

I think we want to fix vercel/styled-jsx#22 before releasing the feature.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 58.534% when pulling 8028776 on add/styled-jsx into d12502e on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 58.534% when pulling 8028776 on add/styled-jsx into d12502e on master.

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage decreased (-1.4%) to 58.534% when pulling 8028776 on add/styled-jsx into d12502e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 58.534% when pulling 6de29b0 on add/styled-jsx into d12502e on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 58.534% when pulling 6de29b0 on add/styled-jsx into d12502e on master.

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+1.07%) to 61.037% when pulling 5514949 on add/styled-jsx into d12502e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.02%) to 60.984% when pulling a34cacf on add/styled-jsx into d12502e on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.02%) to 60.984% when pulling a34cacf on add/styled-jsx into d12502e on master.

@rauchg rauchg merged commit a87ef1a into master Dec 19, 2016
@rauchg rauchg deleted the add/styled-jsx branch December 19, 2016 18:42
@gajus
Copy link

gajus commented Jan 15, 2017

Shame I was not aware of this PR. Would have proposed https://github.com/gajus/babel-plugin-react-css-modules. It incorporates CSS modules and it could be used to inline the styles (gajus/babel-plugin-react-css-modules#2).

Would there be any interest for a PR?

@rauchg
Copy link
Member

rauchg commented Jan 15, 2017

@gajus how do you keep track of detaching styles in a situation like this:

import React from 'react';
import styles from './table.css';

export default class Table extends React.Component {
  render () {
    return <div className={styles.table}>
      <div className={styles.row}>
        <div className={styles.cell}>A0</div>
        <div className={styles.cell}>B0</div>
      </div>
    </div>;
  }
}

?

@gajus
Copy link

gajus commented Jan 16, 2017

@gajus how do you keep track of detaching styles in a situation like this:

Please provide more details. Not sure of the implications of the above code example to https://github.com/gajus/babel-plugin-react-css-modules.

The above example would be written as the following using babel-plugin-react-css-modules:

import React from 'react';
import './table.css';

export default () => {
  return <div styleName='table'>
    <div styleName='row'>
      <div styleName='cell'>A0</div>
      <div styleName='cell'>B0</div>
      <div styleName='cell'>C0</div>
    </div>
  </div>;
};

or as:

import React from 'react';
import table from './table.css';

export default () => {
  return <div styleName='table.table'>
    <div styleName='table.row'>
      <div styleName='table.cell'>A1</div>
      <div styleName='table.cell'>B1</div>
      <div styleName='table.cell'>C1</div>
    </div>
  </div>;
};

Depending on user preference.

@rauchg
Copy link
Member

rauchg commented Jan 16, 2017

@gajus

If the component is not included in the page, doesn't the CSS live on, attached to the <head> of the page?

Since Next is designed around lazy-loading any number of pages throughout a user's session, styled-jsx was designed around the constraint that if the component gets detached, the styles get detached.

We use a technique similar to reference counting to ensure there's at most one style present per component, and that it disappears if the component ceases to exist.

@gajus
Copy link

gajus commented Jan 16, 2017

Oh, I see what you are referring to.

There is no such attempt being made at the moment. It isn't even possible. Style names can be generated dynamically, e.g. <div styleName={Math.random() > 0.5 ? 'a' : 'b'}>C1</div>. If we'd remove the dynamic style resolution, then it would be do-able. However, a lot of people do use runtime style resolution.

Is that a big problem though?

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
ForsakenHarmony pushed a commit that referenced this pull request Jul 25, 2024
This adds support for resolving sub-modules into different but similarly-named modules. Before, `import "foo/bar";`  would always try to resolve "./bar" in the first "foo" package we could find, be it in `node_modules` or TS's `compilerOptions.baseUrl`. Now, we'll try to resolve it in all "foo" packages we can find.

Fixes #420
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.

4 participants