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

Core: Provide a way to load Prism from inside a Worker without listen… #1188

Merged
merged 1 commit into from
Nov 5, 2017

Conversation

Golmote
Copy link
Contributor

@Golmote Golmote commented Sep 13, 2017

…ing to messages.

This should fix #1059 and #1172.

Usage from inside a worker:

var Prism = {
	insideWorker: true
};
self.importScripts('prism.js');

Does that look good to you @ligaz @Rob--W @falsandtru?

I feel the naming is quite poor, but I couldn't come with any better name for this param.

@Golmote Golmote force-pushed the in-worker-fix branch 2 times, most recently from 7b50d60 to 7e767f8 Compare September 13, 2017 20:17
@Rob--W
Copy link
Contributor

Rob--W commented Sep 17, 2017

LGTM. How about replacing insideWorker with a name that more accurately describes the purpose or effect of the option? E.g. disableMessageHandler: true?

@ligaz
Copy link

ligaz commented Sep 18, 2017

disableMessageHandler does not ring a bell to me what exactly it is supposed to do. insideWorker although not perfect makes more sense.

Other alternatives that I can come up with are: runInWorker, inWorker, or just worker, but I'm not sure they are better.

@Golmote
Copy link
Contributor Author

Golmote commented Oct 10, 2017

@LeaVerou @zeitgeist87 Any advice on the naming? I'm receiving mixed signals there. :')

@LeaVerou
Copy link
Member

Can't we autodetect this instead of requiring some special parameter? I believe worker environments can be detected, so why do we need this?

@Golmote
Copy link
Contributor Author

Golmote commented Oct 11, 2017

@LeaVerou We must differentiate two cases where Prism might find itself inside a worker.

  1. When Prism itself creates a Worker for async highlighting.
  2. When the user decides to load Prism from inside a Worker.

The core autodetects when it's in a Worker, but it only handles case 1. We need a param to handle explicitely case 2, so that we can skip attaching the listener for messages.

@LeaVerou
Copy link
Member

I see. Can we instead explicitly handle case 1 somehow? That way including Prism from a worker would just work without any special syntax, and we can use whatever special syntax we want inside Prism when we create the worker for async highlighting.
Not that I can think of a way to do this right now, but maybe you have an idea?

@Rob--W
Copy link
Contributor

Rob--W commented Oct 11, 2017 via email

@Golmote
Copy link
Contributor Author

Golmote commented Oct 19, 2017

@Rob--W Unless I missed something in your explanation, I can't see how this will help us differentiate when Prism is loaded as a worker and when it's loaded with importScripts. In both cases, location will be an instance of WorkerLocation, won't it?

@LeaVerou No, I can't think of any way to differentiate the two automatically.

@Rob--W
Copy link
Contributor

Rob--W commented Oct 23, 2017

@Rob--W Unless I missed something in your explanation, I can't see how this will help us differentiate when Prism is loaded as a worker and when it's loaded with importScripts. In both cases, location will be an instance of WorkerLocation, won't it?

In both cases location is an instance of WorkerLocation indeed. But that is besides the point. I explicitly referred to location as WorkerLocation to emphasize that this is a relatively new API, so that if you want to use it, then you'd better include a fallback in case the location global isn't available.

That having said, when loaded as a worker by Prism, location.pathname.endsWith('/prism.js') is most likely true (unless renamed). When loaded as a library, the condition is most likely false. This difference is how you can reasonably accurately guess whether Prism is used as a stand-alone worker, or as a library in a Worker.

@Golmote
Copy link
Contributor Author

Golmote commented Oct 23, 2017

@Rob--W Oh, ok, I get it now. That's smart, I don't know if it's a reliable enough solution to be considered the default behaviour, though.
@LeaVerou Considering that loading Prism from inside a worker is probably an edge case already, I think requiring a manual configuration is not that annoying.

@LeaVerou
Copy link
Member

Ok, I trust your judgement @Golmote :)
And you're right, I guess it's not that common.

@Golmote Golmote merged commit d09982d into PrismJS:gh-pages Nov 5, 2017
@Golmote
Copy link
Contributor Author

Golmote commented Nov 5, 2017

Ok, I went for the long explicit one: disableWorkerMessageHandler.

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.

4 participants