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

[Request] Add Deno compatibility #3208

Closed
oscarotero opened this issue May 30, 2021 · 8 comments
Closed

[Request] Add Deno compatibility #3208

oscarotero opened this issue May 30, 2021 · 8 comments
Labels
enhancement An enhancement or new feature parser

Comments

@oscarotero
Copy link

Is your request related to a specific problem you're having?
I'd like to use this library in Deno (version 11.0.0). Due it uses standard ES modules, it should work fine, but there are some imports that are failing. For example here

Importing the code in Deno with:

import hljs from "https://cdn.jsdelivr.net/gh/highlightjs/highlight.js@11.0.0-beta1/src/highlight.js";

It throws the following error:

Download https://cdn.jsdelivr.net/gh/highlightjs/highlight.js@11.0.0-beta1/src/highlight.js
TypeError: Unmapped bare specifier "deep-freeze-es6"

This is because in Deno (and standard ES modules) all modules must be urls to files and this is a npm dependency.

The solution you'd prefer / feature you'd like to see added...

Due this is a very simple dependency, I propose to copy the dependency code to this repo, so it can be imported like any other ES module. It coud have its own file or be included in the lib/utils.js:

import deepFreeze from './lib/utils.js';

Any alternative solutions you considered...

Maybe in the cdn-release repo, include also a ESM version with all extenal CJS dependencies converted to ESM.

Additional context...

FYI, I'm using lighlight.js in this plugin to highlight automatically all codes of html pages. I'm using a ESM conversion of v10 (with jspm.dev), but now that v11 is almost compatible, it would be awesome to make these changes so Deno users can import directly the original code.

Thanks!

@oscarotero oscarotero added enhancement An enhancement or new feature parser labels May 30, 2021
@joshgoebel
Copy link
Member

joshgoebel commented May 30, 2021

https://cdn.jsdelivr.net/gh/highlightjs/highlight.js@11.0.0-beta1/src/highlight.js

Our repository is not intended to be used "bare" (it needs to be built first)... you should likely be using the cdn-release or the main NPM package... Did you try using the npm v11 beta package via ESM loading?

Maybe in the cdn-release repo, include also a ESM version with all extenal CJS dependencies converted to ESM.>

Our web builds always have no external dependencies. Pretty sure this would then be a dup of #3199, yes?

It's possible @aduh95 may pick this up and run with it now that it's clear we're not going that direction with our main NPM package, not sure...

@oscarotero
Copy link
Author

oscarotero commented May 30, 2021

Our repository is not intended to be used "bare"

I know, but due Deno is designed to avoid the building phase and import directly the source code, It would be a shame not to take advantage of it. Specially in a project like this that is almost 100% ESM excepting a couple of cases.

This would give us more freedom to load and use the library, because the built version contain all code in a single file, and by keeping everything in different files, we can import only the code we need.

@joshgoebel
Copy link
Member

joshgoebel commented May 30, 2021

but due Deno is designed to avoid the building phase and import directly the source code

Designed... other than that it breaks whenever you have any actual npm dependencies. :-)

because the built version contain all code in a single file

Not at all. Our current builds include core (no languages), common, all, etc... pick and choose what you want. The core highlighting engine is not modular (in any usable sense that we wish to expose), so there is no need to use it in smaller pieces.

What you are really asking for is the ESM core web build, which would be covered by #3199. No one has ever wanted such a web build before... our default web build is common... but I see no issue with having a core build in the future beside common.

@joshgoebel
Copy link
Member

joshgoebel commented May 30, 2021

You could also try pointing Deno at the ESM build for Node.js, but I wonder if it might get hung up on the dual-packaging?

This might have been an argument for us solving the ESM-stub/dual packaging issue slightly differently with v11, but that ship has already sailed now... though of course actually "supporting" Deno properly has a whole host of other practical concerns: compatibility issues, CI testing, how can tests actually run at all if there is no npm support, etc...

@aduh95
Copy link
Contributor

aduh95 commented May 30, 2021

Designed... other than that it breaks whenever you have any actual npm dependencies. :-)

FWIW it's possible to handle these in Deno using import maps.

You could also try pointing Deno at the ESM build for Node.js, but I wonder if it might get hung up on the dual-packaging?

With the recent changes there is no longer an ESM version of core.js in the npm package, and Deno would fail to load the CJS version. I'll try to tackle the CDN changes momentarily.

though of course actually "supporting" Deno properly has a whole host of other practical concerns: compatibility issues, CI testing, how can tests actually run at all if there is no npm support, etc...

I have found it surprisingly easy in some of my personal projects actually, all we'd need would be a PR that adds tests for Deno and a GH workflow that runs them.

@joshgoebel
Copy link
Member

joshgoebel commented May 30, 2021

With the recent changes there is no longer an ESM version of core.js in the npm package,

There never was (unless you mean v11 alpha/beta)... There is now a full ESM folder, but per recommendation for dual packages the main touch points simply import the CJS modules... so I dunno what Deno makes of that. Works fine on Node, I'm guessing Deno is much less happy about it.

Someone who wants "just pure ESM" but doesn't care about Node at all is why #3199 was opened. Whether or not such a build would work for Deno I don't know - but I guess it probably would.

... a PR that adds tests for Deno and a GH workflow that runs them.

Well, we have 1000s of tests... so we'd want to run our existing test suite on Deno, not write Deno specific tests. Perhaps that's not what you meant?

all we'd need

Very optimistic. That assumes everything "just works" 100% perfect and no effort is required to troubleshoot or support Deno differences in behavior. Really that list is the bare minimum we'd need to decide whether Deno was possible to properly declare as supported or not. Not sure if that's something you or others in the Deno community would want to invest time in or if it makes more sense to invest time on the pure ESM "web" build... and then if that works for Deno, so be it - albeit not technically fully supported.

@oscarotero
Copy link
Author

IMHO, a "pure ESM" (that works in both browser and Deno) would be the best solution. I'm not asking for a "Deno version", but a ESM version that works for Deno.

That why I suggested that this repo could become that pure ESM version, so no need to built a different one, but if you think it's better to have it in the CDN repo, that's ok to me too.

@joshgoebel
Copy link
Member

joshgoebel commented May 30, 2021

That why I suggested that this repo could become that pure ESM version

We're quite happy with providing build assets for people and prefer that approach to our distributables. The repository source code is more for "internal use", those building from source, doing very custom things, etc - it is not considered a distributable. Perhaps that will change one day in the shiny new ESM future, time will tell...

I'm not asking for a "Deno version", but a ESM version that works for Deno.

Ah I was reading a bit much into the title of your issue perhaps... if this is really merely "please provide a perfectly pure ESM" version (without the distractions of Node) then it is really a dup of #3199 and I will close it as such.

Lets focus on #3199. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature parser
Projects
None yet
Development

No branches or pull requests

3 participants