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

Move @import rules to an extra/separate browser tag inside tags #1577

Merged
merged 4 commits into from Mar 7, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,8 @@ All notable changes to this project will be documented in this file. If a contri

## Unreleased

- Fix `@import` rules not being enforced to appear at the beginning of stylesheets (see [#1577](https://github.com/styled-components/styled-components/pull/1577))

- Fix StyleTags toElement outputting inline CSS which would cause URL encoding (see [#1580](https://github.com/styled-components/styled-components/pull/1580))

## [v3.2.0] - 2018-03-05
Expand Down
14 changes: 14 additions & 0 deletions src/constructors/test/__snapshots__/injectGlobal.test.js.snap
@@ -0,0 +1,14 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`injectGlobal should extract @import rules into separate style tags 1`] = `
"<style data-styled-components=\\"\\">
/* sc-component-id: sc-global-3616276198 */
html{padding:1px;}
/* sc-component-id: sc-a */
.sc-a {} .b{color:green;}
/* sc-component-id: sc-global-1433896746 */
html{color:blue;}</style>
<style data-styled-components=\\"\\">
/* sc-component-id: sc-global-1433896746-import */
@import url('bla');</style>"
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 this style tag be above the other style tags? Or is it only about the first thing in a certain style tag?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mxstbr I could enforce it to come before the style tag that contains the @import actually. I'm not sure whether we'd want that, since it's mostly meant for fonts only anyway.

But I could modify the makeStyleTag method to allow me to inject elements before another

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I mean if it works that's totally fine!

`;
13 changes: 13 additions & 0 deletions src/constructors/test/injectGlobal.test.js
Expand Up @@ -76,4 +76,17 @@ describe('injectGlobal', () => {
}
`)
})

it('should extract @import rules into separate style tags', () => {
injectGlobal`html { padding: 1px; }`
const Comp = styled.div`color: green;`
shallow(<Comp />)
injectGlobal`html { color: blue; } @import url('bla');`

const style = Array.from(document.querySelectorAll('style'))
.map(tag => tag.outerHTML)
.join('\n')

expect(style).toMatchSnapshot()
});
});
92 changes: 85 additions & 7 deletions src/models/StyleTags.js
Expand Up @@ -20,7 +20,7 @@ import {

import {
sheetForTag,
safeInsertRules,
safeInsertRule,
deleteRules,
} from '../utils/insertRuleHelpers'

Expand Down Expand Up @@ -143,10 +143,24 @@ const getIdsFromMarkersFactory = (markers: Object) => (): string[] =>
Object.keys(markers)

/* speedy tags utilise insertRule */
const makeSpeedyTag = (el: HTMLStyleElement): Tag<number> => {
const makeSpeedyTag = (
el: HTMLStyleElement,
makeExtraTag: ?() => Tag<any>
): Tag<number> => {
const names: Names = Object.create(null)
const markers = Object.create(null)
const sizes: number[] = []
const extractImport = makeExtraTag !== undefined
let extraTag: Tag<any>

const getExtraTag = () => {
if (extraTag === undefined && extractImport) {
// $FlowFixMe
extraTag = makeExtraTag()
}

return extraTag
}

const insertMarker = id => {
const prev = markers[id]
Expand All @@ -164,7 +178,27 @@ const makeSpeedyTag = (el: HTMLStyleElement): Tag<number> => {
const marker = insertMarker(id)
const sheet = sheetForTag(el)
const insertIndex = addUpUntilIndex(sizes, marker)
sizes[marker] += safeInsertRules(sheet, cssRules, insertIndex)

let injectedRules = 0
const importRules = []
const cssRulesSize = cssRules.length

for (let i = 0; i < cssRulesSize; i += 1) {
const cssRule = cssRules[i]
let mayHaveImport = extractImport /* @import rules are reordered to appear first */
if (mayHaveImport && cssRule.indexOf('@import') !== -1) {
importRules.push(cssRule)
} else if (safeInsertRule(sheet, cssRule, insertIndex + injectedRules)) {
mayHaveImport = false
injectedRules += 1
}
}

if (importRules.length > 0) {
getExtraTag().insertRules(`${id}-import`, importRules)
}

sizes[marker] += injectedRules /* add up no of injected rules */
addNameForId(names, id, name)
}

Expand All @@ -178,6 +212,10 @@ const makeSpeedyTag = (el: HTMLStyleElement): Tag<number> => {
deleteRules(sheet, removalIndex, size)
sizes[marker] = 0
resetIdNames(names, id)

if (extraTag !== undefined) {
extraTag.removeRules(`${id}-import`)
}
}

const css = () => {
Expand Down Expand Up @@ -211,9 +249,23 @@ const makeSpeedyTag = (el: HTMLStyleElement): Tag<number> => {
}
}

const makeBrowserTag = (el: HTMLStyleElement): Tag<Text> => {
const makeBrowserTag = (
el: HTMLStyleElement,
makeExtraTag: ?() => Tag<any>
): Tag<Text> => {
const names = Object.create(null)
const markers = Object.create(null)
const extractImport = makeExtraTag !== undefined
let extraTag: Tag<any>

const getExtraTag = () => {
if (extraTag === undefined && extractImport) {
// $FlowFixMe
extraTag = makeExtraTag()
}

return extraTag
}

const makeTextNode = id => document.createTextNode(makeTextMarker(id))

Expand All @@ -230,18 +282,42 @@ const makeBrowserTag = (el: HTMLStyleElement): Tag<Text> => {
}

const insertRules = (id, cssRules, name) => {
insertMarker(id).appendData(cssRules.join(' '))
const marker = insertMarker(id)
const importRules = []
const cssRulesSize = cssRules.length

for (let i = 0; i < cssRulesSize; i += 1) {
const rule = cssRules[i]
let mayHaveImport = extractImport
if (mayHaveImport && rule.indexOf('@import') !== -1) {
importRules.push(rule)
} else {
mayHaveImport = false
const separator = i === cssRulesSize - 1 ? '' : ' '
marker.appendData(`${rule}${separator}`)
}
}

addNameForId(names, id, name)

if (importRules.length > 0) {
getExtraTag().insertRules(`${id}-import`, importRules)
}
}

const removeRules = id => {
const marker = markers[id]
if (marker === undefined) return

/* create new empty text node and replace the current one */
const newMarker = makeTextNode(id)
el.replaceChild(newMarker, marker)
markers[id] = newMarker
resetIdNames(names, id)

if (extraTag !== undefined) {
extraTag.removeRules(`${id}-import`)
}
}

const css = () => {
Expand Down Expand Up @@ -335,9 +411,11 @@ export const makeTag = (
if (IS_BROWSER && !forceServer) {
const el = makeStyleTag(target, lastEl)
if (DISABLE_SPEEDY) {
return makeBrowserTag(el)
const makeExtraTag = () => makeBrowserTag(makeStyleTag(target, el))
return makeBrowserTag(el, makeExtraTag)
} else {
return makeSpeedyTag(el)
const makeExtraTag = () => makeSpeedyTag(makeStyleTag(target, el))
return makeSpeedyTag(el, makeExtraTag)
}
}

Expand Down
22 changes: 1 addition & 21 deletions src/utils/insertRuleHelpers.js
Expand Up @@ -21,7 +21,7 @@ export const sheetForTag = (tag: HTMLStyleElement): CSSStyleSheet => {
}

/* insert a rule safely and return whether it was actually injected */
const safeInsertRule = (
export const safeInsertRule = (
sheet: CSSStyleSheet,
cssRule: string,
index: number
Expand All @@ -42,26 +42,6 @@ const safeInsertRule = (
return true
}

/* insert multiple rules using safeInsertRule */
export const safeInsertRules = (
sheet: CSSStyleSheet,
cssRules: string[],
insertIndex: number
): number => {
/* inject each rule and count up the number of actually injected ones */
let injectedRules = 0
const cssRulesSize = cssRules.length
for (let i = 0; i < cssRulesSize; i += 1) {
const cssRule = cssRules[i]
if (safeInsertRule(sheet, cssRule, insertIndex + injectedRules)) {
injectedRules += 1
}
}

/* return number of injected rules */
return injectedRules
}

/* deletes `size` rules starting from `removalIndex` */
export const deleteRules = (
sheet: CSSStyleSheet,
Expand Down
2 changes: 1 addition & 1 deletion test-results.json

Large diffs are not rendered by default.