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

master branch doesn't work in a web worker #537

Open
marekdedic opened this issue May 20, 2018 · 13 comments
Open

master branch doesn't work in a web worker #537

marekdedic opened this issue May 20, 2018 · 13 comments
Assignees

Comments

@marekdedic
Copy link
Contributor

marekdedic commented May 20, 2018

Hi, I switched to master (from the latest npm version) to get #521, but showdown no longer works in a web worker, it errors with:

Uncaught ReferenceError: require is not defined
    at showdown.js:585

I include it in the web worker with:

importScripts('/node_modules/showdown/dist/showdown.js');
@tivie tivie self-assigned this Jun 5, 2018
@tivie
Copy link
Member

tivie commented Jun 5, 2018

master is unstable. I will look into it as soon as possible.

@marekdedic
Copy link
Contributor Author

Thank you. I have already backported the fix to the latest release for my purposes, but it'd be nice for showdown to work in a Web worker in the future.

@tivie
Copy link
Member

tivie commented Jun 5, 2018

it will, of course.

I actually cannot reproduce the issue you having.

I'm behind schedule for v 2.0 ALPHA 1 release so I'm putting all my effort into it.

I will come back to this issue in the near future and will ask for your help in trying to reproduce it.

@tivie
Copy link
Member

tivie commented Sep 14, 2018

@GenaBitu Can you produce a small example of showdown failing to work in a webworker?

@marekdedic
Copy link
Contributor Author

Hi, I will look into it - maybe it's been fixed in the meantime, we'll see...

@marekdedic
Copy link
Contributor Author

Nope, the issue is still there. (line 587 now). I'll make a minimal example.

@marekdedic
Copy link
Contributor Author

marekdedic commented Sep 19, 2018

Hey, I've created a gist. The showdown file is the current master file dist/showdown.js. Bear in mind, that you can't run this code from a file, Web Workers only work when served from a server...

@marekdedic
Copy link
Contributor Author

From what I understand about modern JS (= not much...), line 587 is only supposed to run if you are in a Node environment, not in the browser. But the way this check is implemented, this check thinks it is in Node when in fact it's a Web Worker.

@tivie
Copy link
Member

tivie commented Sep 19, 2018

ah, it's the JSDOM shim that is breaking the webworker...
That code was provisory but that should be easy to fix.

@marekdedic
Copy link
Contributor Author

Yeah, I've tried commenting out lines 587 and 588, but then 590 breaks because Web workers don't have this.window, which I don't even know why this package would use :)

@tivie
Copy link
Member

tivie commented Sep 19, 2018

Because of the new feature of reverse coverting. We use the DOM parser to parse HTML into Markdown.

@marekdedic
Copy link
Contributor Author

Ah, I didn't know about that feature, thanks. That sounds really cool. A nit-picky question: Why did you decide to include this feature in this package as opposed to making it into its own one? I imagine that the reverse converting is a lot of code - making the package size needlessly bigger for people who don't use it (I would say the majority, but you know, I have a sample size of 1...).
Thanks!

@tivie
Copy link
Member

tivie commented Sep 23, 2018

Mainly for 2 reasons: They share a considerable portion of code and it's harder to maintain 2 libraries instead of one.

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

2 participants