MDX Slide Parsing and CLI #704
Conversation
SunburtReynolds
changed the title
SunburtReynolds
requested review from
VirtualDOMinic,
kiraarghy,
narinluangrath and
rgerstenberger
bin/args.js
Outdated
| .usage(`Usage: spectacle -m <file>`) | ||
|
|
||
| // MDX File | ||
| .option('mdx', { |
There was a problem hiding this comment.
This looks funky to me - Usage: spectacle -m <file>. As a spectacle consumer, I would assume that -m in that scenario means "file name" but here it's actually mdx. A better structure would be to have a filename option like -s/src which takes a file of either .js(x) or .md(x). And then either infer the filetype from webpack or have a boolean flag -m/mdx which sets the filetype to md(x).
It's also weird that the default filename is hidden behind a flag. I would assume you could just run spectacle and have smart defaults like "look for slides.js and spin up webserver"
There was a problem hiding this comment.
What would you expect spectacle -s <jsx file name here> to do? AFAIK that's undefined behavior.
There was a problem hiding this comment.
Run a slideshow from JSX slides. It's currently undefined yes. I think the main point is that the slide file is separate from "is this mdx". And that decision can happen from a user supplied flag, OR we might be able to infer it based on the source file type.
There was a problem hiding this comment.
Makes sense to me. Here's the decision tree I've got in mind:
Did the user supply a file?
If no, default to (?) slides.mdx.
If yes, what kind of file did they supply an mdx file or a jsx file?
If mdx, boot up the presentation.
If jsx (or other), console log about currently unsupported behavior.
There was a problem hiding this comment.
Sounds reasonable, though I'd eventually expect the default to be slides.jsx
|
I'm curious as to why this project has mixed case filenames and directories. My understanding was that we try to use |
|
@rgerstenberger here's another PR to address the |
use kebab-case consistently throughout project
|
Looks good to me! |
| const path = require('path'); | ||
| const HtmlWebpackPlugin = require('html-webpack-plugin'); | ||
|
|
||
| /* |
There was a problem hiding this comment.
@SunburtReynolds I think the context config could fix this: https://webpack.js.org/configuration/entry-context/#context
There was a problem hiding this comment.
@narinluangrath unfortunately, I tried context but could not get it to work! If you have the time to try it out and find a particular configuration that doesn't require all the absolute paths to dependencies, I will absolutely take it 🙏
There was a problem hiding this comment.
Aw man, that is really surprising. I'll try it out tonight.
Description
This PR introduces a two new pieces of functionality:
<Slide />s by the presence of---in the MDXwebpack-dev-serverinstance for an arbitrary MDX fileThese two features work hand in hand to simplify how MDX files can be used in Spectacle. Like in Old Spectacle,
---can be used to denote separate slides. This will work separately from the CLI. However, in combination with the CLI, booting up a hot-reloading Spectacle presentation from an MDX file is very straightforward. Once Spectacle is installed globally (ornpm link'd from the cloned repo), a user can run this from any arbitrary directory:spectacle -s <name of file>.The
-sflag tells the CLI that the user wants to boot up a presentation from the provided file. The CLI defaults to looking for a file namedslides.mdx, but the user can give an alternative name/path. For example:spectacle(looks forslides.mdxin the current directory) orspectacle -s ./nested/my-pres.mdx(points Spectacle directly to the MDX file). In the future, other file types might be supported with the-soption. For now, only MDX files are supported.Webpack - when you see the webpack configuration, you might wonder why it requires so many changes. It appears that when webpack is run from a directory outside of the Spectacle project (i.e. as a globally installed CLI tool), it has immense trouble finding the loaders and babel tools that are used in the configuration. The only solution that I found to bypass this problem was to use Node's
pathlibrary to construct paths to the dependencies.MDX Loader - the loader file has comments to explain each step. It takes significant inspiration from the custom MDX loader in Old Spectacle. However, it includes updates that are a necessity born of the
mdx-jsversion that we're using.Type of Change
How Has This Been Tested?
To test new functionality:
npm linkin the spectacle directory~/Documents/test/nestspectacle -s <your mdx file>to boot up the Spectacle presentation