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

Npm node support #34

Closed
wants to merge 11 commits into from
Closed

Npm node support #34

wants to merge 11 commits into from

Conversation

buymeasoda
Copy link

Hi Lea, here's a version of prism.js adapted to provide node and npm support that you might like to check out.

I've added more details in the npm ticket: #12 (comment)

Hope it's useful, and thanks for sharing prism.js :)

@LeaVerou
Copy link
Member

Hi there,

Thanks for the awesome effort you put into porting this to Node!
However, the changes seem a bit too invasive, i.e. a big part of the code is changed just for Node. Perhaps we could keep it in a fork? Alternatively, it would be nice to explore ways to make it need fewer changes. For example, instead of replacing self everywhere, we could have one check at the top which assigns the global object to self, if self is not defined. i.e. something like:

if (typeof self === 'undefined') {
   var self = global;
}

What do you think?

I'd also be very open to the idea of keeping the Node changes in a plugin, even if it means we should add more hooks and make a few changes to the code.

@buymeasoda
Copy link
Author

I agree that the changes aren't ideal. Feels a bit clunky having both code paths in there, especially for the browser use case.

I think it'd be nice if the end result was something like - a Prism base that could be wrapped to generate a dedicated browser version and a dedicated node version, or the plugin / extension approach you mentioned might work well too.

The main change needed in the core is to not have any directly actioned, browser specific calls that would fail in a node context.

I'll have another go at this, and for the next submission I'll also keep all the peripheral changes out (like changing _ and self), which will make the footprint of changes much smaller.

@buymeasoda
Copy link
Author

Unfortunately, we wouldn't be able to use:

if (typeof self === 'undefined') {
   var self = global;
}

For the check inside the main function as the hoisted self ends up becoming undefined, rather than being interpreted as already existing via window.self. Which causes the browser version to try to assign the value of global.

This form works though:

var self = (typeof window !== 'undefined') ? window.self : global

@buymeasoda
Copy link
Author

I've created a new version of this pull request that includes the bare minimum needed to get Prism functioning in node.

The pull request is: #42

@buymeasoda
Copy link
Author

Closing this pull request in favour of a standalone node version.

Reference: #45

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.

2 participants