From fc581a6cd8b75141e4534527c4b6e17854ea9e9e Mon Sep 17 00:00:00 2001
From: Yuki Hattori <yukihattori1116@gmail.com>
Date: Fri, 1 Jun 2018 10:25:15 +0900
Subject: [PATCH] Suppress confusable theme import when tweaking (#33)

* Suppress confusable import in inline style by import suppress plugin

* Update README.md

* [ci skip] Update CHANGELOG.md
---
 CHANGELOG.md                    |  2 ++
 README.md                       |  6 ++++--
 src/postcss/import/suppress.js  | 28 ++++++++++++++++++++++++++++
 src/theme_set.js                | 11 ++++++++---
 test/marpit.js                  | 14 +++++++++++---
 test/postcss/import/suppress.js | 23 +++++++++++++++++++++++
 6 files changed, 76 insertions(+), 8 deletions(-)
 create mode 100644 src/postcss/import/suppress.js
 create mode 100644 test/postcss/import/suppress.js

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 82244884..bcf142fd 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## [Unreleased]
 
+- Suppress confusable theme import when tweaking ([#33](https://github.com/marp-team/marpit/pull/33))
+
 ## v0.0.6 - 2018-05-29
 
 - Add `header` and `footer` directives ([#22](https://github.com/marp-team/marpit/pull/22))
diff --git a/README.md b/README.md
index 46570f4e..6a7e1772 100644
--- a/README.md
+++ b/README.md
@@ -400,9 +400,9 @@ section {
 }
 ```
 
-`@import` follows [CSS rules](https://developer.mozilla.org/en-US/docs/Web/CSS/@import): it must precede all other statements excepted `@charset`.
+`@import` must precede all other statements excepted `@charset`. (It follows [the original specification](https://developer.mozilla.org/en-US/docs/Web/CSS/@import))
 
-> :information_source: The base theme must be added by using `Marpit.themeSet.add(css)` in advance.
+> :information_source: An importing theme must add by using `Marpit.themeSet.add(css)` in advance.
 
 ##### `@import-theme`
 
@@ -422,6 +422,8 @@ section {
 
 `@import-theme` can place on anywhere of the root of CSS, and the imported contents is inserted to the beginning of CSS in order.
 
+> :warning: You cannot import another theme while [tweaking style by using inline `<style>`](#tweak-theme-in-markdown) and [`style` global directive](#style-global-directive).
+
 #### Tweak theme in Markdown
 
 You can tweak the theme's style in Markdown. Marpit parses `<style>` HTML element in the context of as same as a theme.
diff --git a/src/postcss/import/suppress.js b/src/postcss/import/suppress.js
new file mode 100644
index 00000000..70279b77
--- /dev/null
+++ b/src/postcss/import/suppress.js
@@ -0,0 +1,28 @@
+/** @module */
+import postcss from 'postcss'
+import postcssImportParse from './parse'
+
+/**
+ * Marpit PostCSS import suppress plugin.
+ *
+ * Comment out `@import` / `@import-theme` rules that have imported theme.
+ *
+ * This plugin is useful to prevent the inline style's rolled-up theme import by
+ * unexpected order.
+ *
+ * @alias module:postcss/import/suppress
+ * @param {ThemeSet} themeSet ThemeSet instance.
+ */
+const plugin = postcss.plugin('marpit-postcss-import-suppress', themeSet =>
+  postcss([
+    postcssImportParse,
+    css => {
+      css.walk(node => {
+        if (node.marpitImportParse && themeSet.has(node.marpitImportParse))
+          node.replaceWith(`${node.raw('before')}/* ${node.toString()}; */`)
+      })
+    },
+  ])
+)
+
+export default plugin
diff --git a/src/theme_set.js b/src/theme_set.js
index 31d10508..49d8afdb 100644
--- a/src/theme_set.js
+++ b/src/theme_set.js
@@ -2,6 +2,7 @@ import postcss from 'postcss'
 import postcssAdvancedBackground from './postcss/advanced_background'
 import postcssImportReplace from './postcss/import/replace'
 import postcssImportRollup from './postcss/import/rollup'
+import postcssImportSuppress from './postcss/import/suppress'
 import postcssInlineSVGWorkaround from './postcss/inline_svg_workaround'
 import postcssPagination from './postcss/pagination'
 import postcssPrintable from './postcss/printable'
@@ -185,16 +186,20 @@ class ThemeSet {
     if (opts.inlineSVG)
       slideElements.unshift({ tag: 'svg' }, { tag: 'foreignObject' })
 
-    let appendStyle
+    let append
+
     try {
-      appendStyle = postcss.parse(opts.appendStyle, { from: undefined })
+      if (opts.appendStyle)
+        append = postcss([postcssImportSuppress(this)]).process(
+          opts.appendStyle
+        ).css
     } catch (e) {
       // Ignore invalid style
     }
 
     const packer = postcss(
       [
-        appendStyle && (css => css.last.after(appendStyle)),
+        append && (css => css.last.after(append)),
         postcssImportReplace(this),
         opts.printable &&
           postcssPrintable({
diff --git a/test/marpit.js b/test/marpit.js
index 8c874a53..65fce246 100644
--- a/test/marpit.js
+++ b/test/marpit.js
@@ -158,8 +158,15 @@ describe('Marpit', () => {
     })
 
     context('with inlineStyle option in instance', () => {
-      const instance = inlineStyle => new Marpit({ inlineStyle })
-      const markdown = '<style>section { --style: appended; }</style>'
+      const instance = inlineStyle => {
+        const marpit = new Marpit({ inlineStyle })
+
+        marpit.themeSet.add('/* @theme valid-theme */')
+        return marpit
+      }
+
+      const markdown =
+        '<style>@import "valid-theme";\nsection { --style: appended; }</style>'
 
       it('keeps inline style in HTML when inlineStyle is false', () => {
         const rendered = instance(false).render(markdown)
@@ -169,12 +176,13 @@ describe('Marpit', () => {
         assert(!rendered.css.includes('--style: appended;'))
       })
 
-      it('appends style to css when inlineStyle is true', () => {
+      it('appends style to css with processing when inlineStyle is true', () => {
         const rendered = instance(true).render(markdown)
         const $ = cheerio.load(rendered.html)
 
         assert($('style').length === 0)
         assert(rendered.css.includes('--style: appended;'))
+        assert(rendered.css.includes('/* @import "valid-theme"; */'))
       })
     })
   })
diff --git a/test/postcss/import/suppress.js b/test/postcss/import/suppress.js
new file mode 100644
index 00000000..914784c5
--- /dev/null
+++ b/test/postcss/import/suppress.js
@@ -0,0 +1,23 @@
+import assert from 'assert'
+import postcss from 'postcss'
+import importSuppress from '../../../src/postcss/import/suppress'
+
+describe('Marpit PostCSS import suppress plugin', () => {
+  const themeSetStub = new Map()
+  themeSetStub.set('imported', { css: '' })
+
+  const run = input =>
+    postcss([importSuppress(themeSetStub)]).process(input, { from: undefined })
+
+  it('comments out @import and @import-theme rules with valid theme', () =>
+    run('@import "imported";\n@import-theme "imported";').then(({ css }) =>
+      assert(
+        css === '/* @import "imported"; */\n/* @import-theme "imported"; */'
+      )
+    ))
+
+  it('ignores @import and @import-theme rules with invalid theme', () => {
+    const style = '@import "invalid";\n@import-theme "invalid";'
+    return run(style).then(({ css }) => assert(css === style))
+  })
+})