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

basic node compatibity mode #12295

Closed
6 of 7 tasks
ry opened this issue Oct 1, 2021 · 21 comments
Closed
6 of 7 tasks

basic node compatibity mode #12295

ry opened this issue Oct 1, 2021 · 21 comments
Assignees

Comments

@ry
Copy link
Member

ry commented Oct 1, 2021

Make the following work:

mkdir deno-project && cd deno-project && yarn init deno-project -y
yarn add rollup
echo 'import { VERSION } from "rollup";' > foo.mjs
echo 'console.log(VERSION);' >> foo.mjs
deno run --compat foo.mjs
@happenslol
Copy link

Hey,
tsserver has annoyed me to a point where I'd do anything to have a better/faster alternative, including tackling this issue ;-)

Is there currently any work being done on this, or has someone else already picked up this issue and could give me some pointers on where I could help? Simply resolving packages from the local node_modules folders seems pretty straightforward to me, even though this just covers packages with an explicit module field.
Down the road it would also be interesting to look into handling yarn v2 import since it also uses a central package folder instead of local node_modules (which is similar to what deno does afaik?).

@wojpawlik
Copy link

Compatibility mode should be enabled per-import, so that users aren't forced to "poison" entire process for a single Node-only dependency.


CJS and extensionless imports are on the way out: https://blog.sindresorhus.com/hello-modules-d1010b4e777b. https://deno.land/std/node/import_map.json would simplify writing or generating ESM that works on both runtimes.

@bartlomieju
Copy link
Member

Compatibility mode should be enabled per-import, so that users aren't forced to "poison" entire process for a single Node-only dependency.

That sounds nice, but is not really feasible to apply different resolution schemes per import.

CJS and extensionless imports are on the way out: https://blog.sindresorhus.com/hello-modules-d1010b4e777b.

I don't believe that's true. It will take years before CJS are abandoned by community and few more to be removed from Node (if ever).

@kt3k
Copy link
Member

kt3k commented Oct 5, 2021

load file corresponding to "module" entry

Do we only use "module" property? I believe "module" property is a non-standard property used by some bundlers like rollup, webpack, etc, and most npm packages don't provide it.

Another question: Do we provide global require function?

@AnInternetTroll
Copy link
Contributor

Could the flag name be changed from --compat to anything else? IMO it's not descriptive and it should better reflect that it's a compatibility mode with node.js.

@wojpawlik
Copy link

--nodejs-compat?

@bartlomieju
Copy link
Member

Thinking about:

Figure out X-Deno-Warning coming from Node built-in mappings

I'm leaning towards bundling std/node in the binary. We already have deno_std as a submodule in the tree, so it should be trivial to bundle that code during build. It would get solve the versioning problem (of using trunk version) and instead it would used pinned version. Unfortunately it means we'd have to keep deno_std very much up-to-date in the tree and users if we wanted users to have the latest version; even then it would be at least one version behind latest "deno_std".

Curious to hear other ideas.

@hayd
Copy link
Contributor

hayd commented Oct 6, 2021

optionally passing the std version (with a recent default that could be overridden), would at least enable node/std to update independently of deno.

e.g. --compat be equivalent to --compat=0.110.0 etc. 😬
edit: or perhaps using the url might be preferable --compat='https://deno.land/std@0.110.0/node/'
(that way, for example, users could test fixes to std node, or roll their own for whatever reason.)

update: #12508 makes this an env variable

@vwkd
Copy link
Contributor

vwkd commented Oct 6, 2021

Unfortunately it means we'd have to keep deno_std very much up-to-date in the tree and users if we wanted users to have the latest version; even then it would be at least one version behind latest "deno_std".

Does it need to be one version behind? Why not release deno_std before deno, update the deno tree, then release deno...?

@wojpawlik
Copy link

Figure out X-Deno-Warning coming from Node built-in mappings

I'm leaning towards bundling std/node in the binary.

Why not simply pin a deno_std version on CLI release instead of always requesting latest?

@bartlomieju
Copy link
Member

Unfortunately it means we'd have to keep deno_std very much up-to-date in the tree and users if we wanted users to have the latest version; even then it would be at least one version behind latest "deno_std".

Does it need to be one version behind? Why not release deno_std before deno, update the deno tree, then release deno...?

That's the current release workflow, additionally an update to dotland repo needs to be merged before CLI and std are available for discovery.

Figure out X-Deno-Warning coming from Node built-in mappings

I'm leaning towards bundling std/node in the binary.

Why not simply pin a deno_std version on CLI release instead of always requesting latest?

Because it would have to be one version behind as mentioned above.

@kt3k
Copy link
Member

kt3k commented Oct 8, 2021

I think we should keep Object.prototype.__proto__ in compat mode. In my previous experiment with std/node, I found some heavily dependent npm modules (such as chalk, acorn-node, etc) depend on that feature. Keeping __proto__ should improve the compatibility significantly

@bartlomieju
Copy link
Member

I think we should keep Object.prototype.__proto__ in compat mode. In my previous experiment with std/node, I found some heavily dependent npm modules (such as chalk, acorn-node, etc) depend on that feature. Keeping __proto__ should improve the compatibility significantly

I'm not a fan of that, we'd need to branch our "runtime setup" code to achieve that. @kitsonk do you have an opinion?

@kitsonk
Copy link
Contributor

kitsonk commented Oct 11, 2021

It is a very bad idea. The number of security advisories that are related to __proto__. It has been dropped from the spec. I would be curious to know why current versions of chalk and acorn require access and what we can do to support them without reintroducing a security issue, even in a compat mode.

@andreubotella
Copy link
Contributor

It is a very bad idea. The number of security advisories that are related to __proto__. It has been dropped from the spec. I would be curious to know why current versions of chalk and acorn require access and what we can do to support them without reintroducing a security issue, even in a compat mode.

It hasn't been dropped from the spec, it was moved out of annex B and made normative optional: tc39/ecma262#2125

@bartlomieju
Copy link
Member

It is a very bad idea. The number of security advisories that are related to __proto__. It has been dropped from the spec. I would be curious to know why current versions of chalk and acorn require access and what we can do to support them without reintroducing a security issue, even in a compat mode.

I checked chalk and couldn't find a single reference to it, as well as its dependencies. I suggest we punt on it, and reevaluate if it becomes a huge pain point; I'm skeptical anyone will hit it in the first weeks after introducing of --compat.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 12, 2021

It is a very bad idea. The number of security advisories that are related to __proto__. It has been dropped from the spec. I would be curious to know why current versions of chalk and acorn require access and what we can do to support them without reintroducing a security issue, even in a compat mode.

It hasn't been dropped from the spec, it was moved out of annex B and made normative optional: tc39/ecma262#2125

Apologies... I just remember the debate, forgot it was moved to Annex B. I got confused on the effort to remove everything from Annex B, but that it was a compromise to move this to Annex B. Either way, it shouldn't be there and shouldn't be used.

@petamoriken
Copy link
Contributor

How about adding another flag that can only be used in compat mode, like --compat --enable-proto?

NOTE: Node.js has a --disable-proto=mode option

@ije
Copy link

ije commented Oct 14, 2021

Can we change the user-agent for remote modules too when the --compat flag passed?for example Deno/1.15.1 (compat), with this the CDN (esm.sh/sykpack) can have better compatibility!

@ry
Copy link
Member Author

ry commented Oct 22, 2021

Can we change the user-agent for remote modules too when the --compat flag passed?for example Deno/1.15.1 (compat), with this the CDN (esm.sh/sykpack) can have better compatibility!

That's an interesting idea. I think we should let compat mode settle down a bit first. We're still actively working on it. Ideally compat mode would not require any changes on the cdns.

@ry
Copy link
Member Author

ry commented Oct 28, 2021

Basic compat mode is working. Closing this issue and moving to #12577

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