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

Setup for sourceMap support for dev environment #2101

Closed
wants to merge 18 commits into from

Conversation

yiochen
Copy link

@yiochen yiochen commented Oct 16, 2018

I created a new PR with v4 merged into my branch
This is WIP

I created this merge request to get feedback on my approach. Let me know if I missed anything.

As discussed in this issue, the source map support will only be available for dev environment for two reasons:

  1. CSS sourcemap will greatly increase the bundle size of the app
  2. SpeedyTag.insertRule doesn't insert comment, sourceMap needs to live inside a comment

Changes need to be made to both the styled-components repo and the babel-plugin-styled-components repo

  1. We need to render the styles into style tag
  2. Each style tag can only have one sourceMap, because styles are dynamically generated and added to the style tag, we need to make sure one style tag contains one rule.
  3. The only source mapping we can do is to map the begining of a rule to the beginng of the styled-component code. We can't do mapping for each css property.

In this repo (with this merge request), I added support to pass sourceMap into styled component with withConfig

styled.div.withConfig({
    sourceMap: `/*# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIk9yaWdpbmFsLmpzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUNBIiwiZmlsZSI6ImRpc3QuanMiLCJzb3VyY2VzQ29udGVudCI6WyIvLyBUaGlzIGxpbmUgd2lsbCBiZSBza2lwcGVkXG5JIHdpbGwgc2V0IGNzcyBjbGFzcyBoZXJlXG5JIHdpbGwgc2V0IGNvbG9yIGhlcmVcbiJdfQ== */`
})`
    color: red
`

This should generate a style tag that contains rules only for this component

<style>
/* sc-component-id: sc-a*/
.sc-a {
  color: red;
} /*# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIk9yaWdpbmFsLmpzIl0sIm5hbWVzIjpbXSwibWFwcGluZ3MiOiJBQUNBIiwiZmlsZSI6ImRpc3QuanMiLCJzb3VyY2VzQ29udGVudCI6WyIvLyBUaGlzIGxpbmUgd2lsbCBiZSBza2lwcGVkXG5JIHdpbGwgc2V0IGNzcyBjbGFzcyBoZXJlXG5JIHdpbGwgc2V0IGNvbG9yIGhlcmVcbiJdfQ== */
</style>

The second part of the work would be done in babel plugin repo to generate those sourceMap comment.

Because this uses withConfig, looks like injectGlobal and keyframe don't support it.
Edit: Since we are switching to createGlobalStyle, I will take a look at if I can add sourcemap to it

Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Overall this approach makes sense to me, I'm excited to see where this'll go!

@quantizor
Copy link
Contributor

I published an experimental version of this under styled-components@sourcemap so you can test it in conjunction with the changes you make to the babel plugin. I'd prefer to not merge this until both are done to make sure no further changes are needed on this side.

@yiochen
Copy link
Author

yiochen commented Nov 19, 2018

Got some progress:
sourcemap
It's not complete yet. Currently, server rendered style doesn't have sourcemap because of a recent change to improve rehydration. I will take a look at it in the next few days.
Also the bundle size failed CI again.

@mxstbr
Copy link
Member

mxstbr commented Nov 19, 2018

Amazing! 😍😍😍 Can't wait to land this 👏

@quantizor
Copy link
Contributor

Also the bundle size failed CI again.

Just make sure all the code is inside NODE_ENV !== 'production' checks and it should be omitted

@yiochen yiochen changed the title Setup for sourceMap support for dev environment - rebased Setup for sourceMap support for dev environment Nov 24, 2018
@yiochen
Copy link
Author

yiochen commented Nov 25, 2018

sourcemap2
Should be ready for code review. The babel plugin PR is at styled-components/babel-plugin-styled-components#180

@yiochen
Copy link
Author

yiochen commented Nov 25, 2018

Looks like I haven't handled some edge cases. I will work on them

@yiochen
Copy link
Author

yiochen commented Nov 25, 2018

I tested it in Chrome, Firefox and Safari. Looks like only Chrome works. Emotion's sourcemap work with Chrome and Firefox, but not Safari either.

Chrome is very lenient with the mapping in sourcemap. For a generated style tag like the following
screen shot 2018-11-25 at 2 18 00 pm
Seems like as long as there is a sourceMappingURL comment in the style tag, Chrome will use the sourcemap even if the position doesn't quite match.
In the screenshot, I have a simple component

const Container = styled.div`
  background: ${props => props.background};
  color: white;
`;

In the sourcemap comment, I mapped the first line, first character (start of the comment /* sc-component ...) to the source code. Chrome was able to map .exOHhY class to it.

When the prop changes, styled-components will add a new class in the same style tag
screen shot 2018-11-25 at 2 22 44 pm
Chrome was able to use the same sourcemap for the new class (qrATe) as well.

While Firefox is stricter with sourcemap mapping. The mapping has to match the exact position of the classname in the style tag. This imposes two issues:

  1. new classes for the same component will not get mapping if we append it at the end of the previous class (emotion solve this by creating a new tag for the new class)
  2. looks like Firefox doesn't respect line break in a textnode, so a style
/* sc-component-id: src__Container-on3mkn-0 */
.exOHhy{background: pink;}

becomes /* sc-component-id: src__Container-on3mkn-0 */ .exOHhy{background: pink;}
I would need to calculate the length of the marker (/* sc-component-id: src__Container-on3mkn-0 */) to know the starting position of .exOHhy

I will think about the solution. If anybody has any question or suggestion, let me know.

@mxstbr mxstbr requested a review from kitten November 26, 2018 08:57
@kitten
Copy link
Member

kitten commented Dec 8, 2020

Closing since this has gone very stale for now. I don't think there's been a big need or shout from CSS-in-JS users, calling this a must-have anymore, as browser's CSS devtools get better. But we'll observe and see what happens to this.

@kitten kitten closed this Dec 8, 2020
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.

None yet

4 participants