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

Fix: Add ability to reference external themes (closes #183) #571

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robertjdominguez
Copy link

Description

I like Marp, but I don't particularly like using VS Code. As a user of the CLI, I'd like to be able to write my markdown in any editor and then reference a hosted stylesheet to use as a particular theme. This is common — as an example — for my organization, as we want to use a single source of truth for our template(s).

Implementation

In the config.ts, we don't try to resolve the local path if http is included within the string passed to the --theme argument:

path: this.args.theme.includes(`http`)
            ? this.args.theme
            : path.resolve(this.args.theme),
        }

Further, on the load() function, we either use the local file via a buffer, or fetch the remote file:

async load() {
    if (this.isUrl(this.filename)) {
      // Fetch the content from a remote URL
      const response = await fetch(this.filename)
      this.readBuffer = Buffer.from(await response.text())
    } else {
      // Read the content from a local file
      this.readBuffer = await fs.promises.readFile(this.filename)
    }
  }

  private isUrl(filename: string): boolean {
    try {
      new URL(filename)
      return true
    } catch {
      return false
    }
  }

Testing

Coverage was above the 95% threshold 🎉

You can use this ref when building the binary:

<PATH_TO_BINARY> --theme https://raw.githubusercontent.com/hasura/marp-template/main/custom-default.css -- <PATH_TO_DECK>

Copy link
Member

@yhatt yhatt left a comment

Choose a reason for hiding this comment

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

Some concerns:

  • theme field from the configuration file should recognize URL as same as --theme CLI option.
  • How about in --theme-set CLI options / themeSet field in the configuration file?
  • Compatibillity with watch mode: In ThemeSet class, the URL should except from the observing files.

const fn = [...baseFn, ...themes.map((t) => t.filename)]

@@ -3,7 +3,7 @@ import fs from 'fs'
import path from 'path'
import { Marpit } from '@marp-team/marpit'
import { isDynamicPattern } from 'globby'
import { warn } from './cli'
import { warn, info } from './cli'
Copy link
Member

Choose a reason for hiding this comment

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

Unused import?

Copy link
Author

Choose a reason for hiding this comment

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

This was from debugging, but I can remove it in the next commit.

Comment on lines +46 to 53
private isUrl(filename: string): boolean {
try {
new URL(filename)
return true
} catch {
return false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

isUrl() may be better to make public class method, and also use for testing whether apply path.resolve to the passed theme. In addition, it might be good to limit the URL protocol to http or https.

static isRemoteUrl(filename: string) {
  try {
    const url = new URL(filename)
    return url.protocol === 'http' || url.protocol === 'https'
  } catch {
    return false
  }
}
{
  path: Theme.isRemoteUrl(this.args.theme) ? this.args.theme : path.resolve(this.args.theme)
}

path: path.resolve(this.args.theme),
path: this.args.theme.includes(`http`)
? this.args.theme
: path.resolve(this.args.theme),
}

if (this.conf.theme)
Copy link
Member

Choose a reason for hiding this comment

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

The path normalization is required also for the theme string from the configuration file. (marp.config.js etc)

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

2 participants