Prism version conflict

[imranolas]

Prism was nailed down to 1.6.0 due to #512, but spectacle still depends on react-live which has a dependency on Prism 1.14.0

│ │ ├─┬ react-live@1.10.1
│ │ │ ├─┬ buble@0.19.3
│ │ │ │ ├── acorn@5.6.2 deduped
│ │ │ │ ├─┬ acorn-dynamic-import@3.0.0
│ │ │ │ │ └── acorn@5.6.2 deduped
│ │ │ │ ├─┬ acorn-jsx@4.1.1
│ │ │ │ │ └── acorn@5.6.2 deduped
│ │ │ │ ├─┬ chalk@2.4.1
│ │ │ │ │ ├── ansi-styles@3.2.1 deduped
│ │ │ │ │ ├── escape-string-regexp@1.0.5 deduped
│ │ │ │ │ └─┬ supports-color@5.4.0
│ │ │ │ │   └── has-flag@3.0.0
│ │ │ │ ├─┬ magic-string@0.22.5
│ │ │ │ │ └── vlq@0.2.3
│ │ │ │ ├── minimist@1.2.0 deduped
│ │ │ │ ├── os-homedir@1.0.2 deduped
│ │ │ │ └── vlq@1.0.0
│ │ │ ├── core-js@2.5.7 deduped
│ │ │ ├─┬ dom-iterator@1.0.0
│ │ │ │ ├── component-props@1.1.1
│ │ │ │ └── component-xor@0.0.4
│ │ │ ├─┬ prismjs@1.14.0

This version conflict leaves me unable to show a code slide in ReasonML. I can't help but wonder if it would be better to just load the languages in the way that the Prism team recommends PrismJS/prism#1394

[ryan-roemer]

@sslotsky -- If your deck is in a public repo, can you point me to it? If not, can you create a minimal repository that exposes the issue?

We should be able to force a resolution correctly, but I need to see the full tree + build configuration to better understand what's going on.

Looking at your links:

• https://github.com/PrismJS/prism/blob/gh-pages/components/index.js -- looks Node.js specific and mucks directly with require.cache which gives me the heeby geebies.

@kenwheeler @kitten -- could we use either of these? Would we want to?

• https://prismjs.com/plugins/autoloader/
• https://prismjs.com/download.html#themes=prism&languages=markup+css+clike+javascript

Once we have a clearer reproduction of the error, we can all chat out best path forward...

[Golmote]

Hi!

Do Spectacle's builds always use Babel? @mAAdhaTTah took the time to think about this and ended up writing a babel plugin for this: https://www.npmjs.com/package/babel-plugin-prismjs Maybe you could take a look at it and see if it can generate an appropriate file taking languages dependencies into account automatically.

[mAAdhaTTah]

@ryan-roemer Yes, the components/index.js is Node-specific and isn't intended to be used in webapps. The cache-clearing is to ensure that Prism modules are reloaded in the correct order so dependencies function correctly.

The intention was definitely to solve this problem with the babel plugin, although I think you might need to use it both in spectacle & react-live (and possibly component-playground, if you're using it there too). The autoloader might also work, but it would require adding a build step in the project to copy all the components into the dist directory and setting the autoloader path at runtime.

[sslotsky]

@ryan-roemer thanks for jumping on this so quickly and for bringing in the Prism folks!

I have a minimal repo here that has the problem. Much appreciated.

[ryan-roemer]

@mAAdhaTTah @Golmote -- Thanks. We do use babel and everything starts at src with ESM, so looks like https://github.com/mAAdhaTTah/babel-plugin-prismjs may be an option. And we can integrate with all of react-live, component-playground and spectacle if needed.

@sslotsky -- I'm slammed today, but having a short repository is invaluable to us. We'll dig into this, verify the error, then assess in code what's actually going wrong, and then most likely work in some of the solutions mentioned above.

[sslotsky]

🙇‍♂️ 🙏 🙌

[mAAdhaTTah]

@ryan-roemer Great! And we're here to help if you need anything. The babel plugin is brand new, so if you run into issues, let us know. I use Spectacle myself, so happy to do what I can.

[ryan-roemer]

Error is verified. Will try to find time this week to dig in more, but I'm on vacation half the week, so may be after...

[sslotsky]

Understood. Any way I can help?

[ryan-roemer]

If someone wants to throw up a WIP / POC PR that could potentially work, that may aide our efforts.

But if we're changing two projects simultaneously the infrastructure stuff might get complicated -- I'm pretty familiar with this type of situation and we've even written a dev tool lank to assist with the shortcomings of npm link and multi-repo dev.

So tl;dr, any work that may help us along is appreciated, but we can probably get it done on our side if we get enough free time in the coming weeks...

[sslotsky]

@mAAdhaTTah I actually took a crack at this on the react-live repo and did run into some trouble trying to run the demo app, which is SSR with next.js. Do you have any examples of using the babel plugin with a React SSR app? I think react-live might be a good place to start since we can test the demo app without having to worry about duplicated dependencies, and also make sure that it works for SSR.

[ryan-roemer]

@sslotsky -- I haven't worked much in the react-live repo, but a lot of us have. As long as there is a babel step for the SSR stuff, I'm guessing we can get something going.

Thanks for the info -- we'll start there first rather than spectacle itself.

[mAAdhaTTah]

@sslotsky SSR shouldn't really impact things. All the babel plugin does is ensure all the dependencies are loaded in the correct order. Prism should otherwise work fine on the server; the highlight method doesn't require the DOM, though the other methods do.

[sslotsky]

@mAAdhaTTah I found that if I just naively follow the example on the babel plugin's README page, I get an error from highlightAll because document is undefined.

I tried to move the highlightAll call to a place where there would surely be a dom (e.g. componentDidMount). I wound up with this:

  //componentWillMount() {
  //  const html = prism(normalizeCode(this.props.code), this.props.language)
  //  this.setState({ html })
  //}

  componentDidMount() {
    Prism.highlightAll();
    const html = prism(normalizeCode(this.props.code), this.props.language)
    this.setState({ html })
    this.recordChange(this.getPlain())
    this.undoTimestamp = 0 // Reset timestamp
  }

The prism function is defined as:

import Prism from 'prismjs';

const prism = (code, language = 'jsx') => Prism.highlight(code, Prism.languages[language])

With this change I get an error in the tokenize method as it seems that grammar is undefined.

Hopefully my mistake is simple?

[sslotsky]

.babelrc

  "plugins": [
    "prismjs", {
      "languages": ["clike", "javascript", "markup", "jsx", "reason"],
      "plugins": ["line-numbers"],
      "theme": "twilight",
      "css": true
    },

[mAAdhaTTah]

Yeah, PrismJS itself assumes a browser(-like) environment. That's not really specific to the babel plugin.

The plugin configuration needs to be in an array:

{
  "plugins": [
    ["prismjs", {
      "languages": ["clike", "javascript", "markup", "jsx", "reason"],
      "plugins": ["line-numbers"],
      "theme": "twilight",
      "css": true
    }] 
  ]
}

More generically speaking, your .babelrc needs to be structured like this:

{
  "plugins": [
    ["prismjs", config] 
  ]
}

[sslotsky]

Good catch, thanks. I made this change and it doesn't seem to help? Here's the languages that are available at runtime.

[mAAdhaTTah]

Do you have a branch / fork I can look at?

[sslotsky]

https://github.com/sslotsky/react-live/tree/babel-prism

[mAAdhaTTah]

When react-live itself is built, it sticks with the import Prism from 'prismjs' bit for the ESM build, which is good, but it means that the prism babel configuration needs to be at the app level, e.g. in the demo. This would allow the consumer of react-live to configure how much of Prism they wants to include in their app.

This also requires consumers to run babel over their entire dependency chain, or at least any dependency that has Prism as a dependency.

Is that a reasonable expectation for consumers?

Also, right now, babel-plugin-prismjs doesn't support CJS, although I don't think adding support would be difficult, but let's sort out the easy case first.

[sslotsky]

Speaking only for myself as a consumer, I think that the extra step of configuring the babel plugin might fall into the annoying-but-tolerable category, as long as the requirements and steps are highly discoverable. A way to automate as much of it as possible would be preferred, or at least have something make a lot of noise if the configuration is missing.

[mAAdhaTTah]

Documentation in all 3 packages, plus default + links in the changelog should cover it. Maybe even update the starter / generator packages.

Yeah, I'll add validation to the babel plugin settings though. It should yell at you.

Those 3 packages will have to remove their current Prism imports, so the consumer can configure it completely. This might be a breaking change for those packages; not sure if there's any way around that 😕.

[imranolas]

I've pinned prismjs to 1.6.0 in react-live and bumped the version as a stopgap for the moment.

@sslotsky I agree that a config would be annoying and I'd worry that forcing users to setup a babel plugin creates friction in what should be a relatively painless experience otherwise.

NB. Some surprising behaviour when using the hub cli - I intended to reference the issue in the PR but instead hub converted the issue into a PR. I didn't even know that was a thing! Sorry everyone!

We can open a new issue to continue the discussion if preferred.

[sslotsky]

@imranolas thanks! Question: willing pinning prism to 1.6.0 allow me to use ReasonML syntax? I have some concerns that whichever Prism loading code that gets executed first/last could take precedent in terms of which languages are actually loaded.

[imranolas]

@sslotsky No problem. I've tested it against your minimal repro and it appears to work but I hadn't considered the issue could be loading precedence. Hopefully this is stable for now until we can find the right solution.

[ryan-roemer]

@sslotsky -- It works provided no other packages you add have a prismjs version that is different and that npm flattening properly collapses to only one used prismjs package. If there are 2+ packages in node_modules tree then this temporary bandaid hack won't work.

But from spectacles POV only, this is sufficient to tide us by until we get a real solution.

[ryan-roemer]

🚢 🤕