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

Version 1.14.0 crashes when used in Spectacle #1394

Closed
sslotsky opened this issue Apr 16, 2018 · 13 comments
Closed

Version 1.14.0 crashes when used in Spectacle #1394

sslotsky opened this issue Apr 16, 2018 · 13 comments

Comments

@sslotsky
Copy link

sslotsky commented Apr 16, 2018

Version 1.14.0 appears to have introduced a breaking change to spectacle, which I initially reported to them but probably really belongs here.

The prism portion of the stacktrace:

Uncaught TypeError: Cannot read property 'buildPlaceholders' of undefined
    at prism-handlebars.js:30
    at Object.run (prism-core.js:451)
    at highlight (prism-core.js:280)

Apparently introduced in this commit.

Please let me know if I can provide any more information. Many thanks for the time and effort.

@Golmote
Copy link
Contributor

Golmote commented Apr 16, 2018

The dependency for PHP has changed in #1367. You need to include the component markup-templating in addition to c-like (and probably markup depending on your use case).

EDIT: Woops, I wrote PHP here, instead of Handlebars. So Handlebars requires the component markup-templating, which requires markup.

@sslotsky
Copy link
Author

Thanks for the reply @Golmote . I'm not totally sure I understand the suggestion. Are you saying that projects that use prism now have additional peer dependencies to satisfy? This seems like a breaking change for consumers of this library, should there have been a major version bump? Perhaps I'm misunderstanding what the issue is?

@mAAdhaTTah
Copy link
Member

@sslotsky The dependency is internal to Prism. Prism languages have dependencies on other Prism languages.

@sslotsky
Copy link
Author

Just to confirm, that means this is a bug with no userland workaround? Just want to be sure I understand. Thanks.

@mAAdhaTTah
Copy link
Member

It's not a bug. However you decide to consume Prism, you need to make sure the languages have their dependencies satisfied. Those dependencies are described here. You can see here that handlebars has a dependency on markup-templating.

@sslotsky
Copy link
Author

I am sorry if I'm being slow, I just really do not understand how this isn't a breaking change. We upgrade a minor version of prism and suddenly our code doesn't work anymore.

@Golmote
Copy link
Contributor

Golmote commented Apr 17, 2018

Hi.

This seems like a breaking change for consumers of this library, should there have been a major version bump?

I've took the time to wonder about this since you posted this issue.

Prism provides no insurance that the dependencies between the components won't change. It does provide the insurance that the files components.json (and components.js) will contain the required information regarding those dependencies.

Furthermore, we usually assume that users using Prism in the browser will get a bundle from the Download page. If they were to upgrade to 1.14 using the "redownload URL" on this page, there would be no problem.
The ones who don't can still use the Autoloader plugin if they don't want to handle the dependencies themselves.
Users using Prism with Node are encouraged to use the loadLanguages() helper function, introduced in 1.13, if they want the dependencies to be automatically handled.

For the rest of the users, the ones who decided, on their own, to manually handle those dependencies, yes 1.14 will break their code, regarding the ERB, Handlebars, PHP and Smarty components.

Taking all of this into account, I don't consider 1.14 a BC-breaking release.

I'm really sorry this introduced an issue on Spectacle's side, yet the way they handle the dependencies is not really realiable. I don't know their use case, but they should probably have come to us (and maybe they still should) so we could discuss to find a better solution.

@sslotsky
Copy link
Author

Thank you @Golmote for the thorough explanation.

@Golmote Golmote closed this as completed Apr 17, 2018
@mAAdhaTTah
Copy link
Member

@sslotsky I might suggest opening an issue on Spectacle's repo to open a line of communication about this issue. We don't really have a good "story" for bundlers right now besides that kind of manual setup, so maybe this is a good opportunity to try and solve that.

@sslotsky
Copy link
Author

Thanks @mAAdhaTTah I actually reported it there first! No activity from them yet but hopefully they respond soon.

@ryan-roemer
Copy link

Hi @Golmote!

If you're still up for it, we're game chatting out a reliable solution for getting a subset of prism deps for Spectacle that we need. Maybe best bet is head over to FormidableLabs/spectacle#546 since it looks like the issue is our method of importing?

For hopefully a useful background, we target CJS + ESM individual files with Babel, and UMD with Webpack.

Thanks!

@Golmote
Copy link
Contributor

Golmote commented Jun 8, 2018

@ryan-roemer Hi, thanks for getting in touch. I'll join the discussion in FormidableLabs/spectacle#546.

@Fi1osof
Copy link

Fi1osof commented Nov 3, 2020

Still have error (v 1.22.0). And do not understand how to fix. Can anybody tell like 'just install this dep:...'?
Thanks!

UPD: i understood. Just add import 'prismjs/components/prism-markup-templating'; before

import prism from 'prismjs';

// Added
import 'prismjs/components/prism-markup-templating';

import 'prismjs/components/prism-php';
import 'prismjs/components/prism-sql';
import 'prismjs/components/prism-smarty';
import 'prismjs/components/prism-jsx';
import 'prismjs/components/prism-graphql';
import withStyles from 'material-ui/styles/withStyles';

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

No branches or pull requests

5 participants