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

[WIP] esm: Import modules by https #21223

Closed
wants to merge 13 commits into from
Closed

[WIP] esm: Import modules by https #21223

wants to merge 13 commits into from

Conversation

viktor-ku
Copy link
Contributor

@viktor-ku viktor-ku commented Jun 8, 2018

Hi everyone,

I implemented basic version of esm over the internet idea. Example below is working.

I'd like to know from the community: what do you think?

Personally I am dying to see Node.js doing something like this. I just feel like node_modules managed by npm sometimes just gets overhead. It's how we got used to it - yeah. But it doesn't mean we can't experiment with other ideas, right?

import assert from 'assert';

import { a } from 'https://raw.githubusercontent.com/kuroljov/esm-test/master/a.js';
import { b } from 'https://raw.githubusercontent.com/kuroljov/esm-test/master/b.js';

assert.strictEqual(a, 'a');
assert.strictEqual(b(), 'b');

How

I basically added new translator called http where I am fetching (or if it's already there then reading from the disk) something

Next

I am able to invest more time to it but I'd like to know if it will make sense. That is why WIP basically 😄

If yes then I'd like to see in this PR

  • Remote dependency resolving (right now you can't fetch script that has import statements (it's like in the browser right now - you get one script that has to export default)
  • Community fixes and suggestions

Checklist

Consider this PR more like a draft for now. Things will get done if I get green light

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the pr! unfortunately this doesn't really fulfill what I think are some important prerequisites for this. 1) we should follow https://html.spec.whatwg.org/multipage/webappapis.html#fetching-scripts and 2) we shouldn't need to use temp files.

because of this i am -1 on this change. I should clarify however that I do want to support importing from https URLs in the future.

@aviov
Copy link

aviov commented Jun 8, 2018

This thing makes sense, like Codesandbox does, for example.
Question: will this work lead to dynamically loaded modules or services in larger networking app?

@devsnek devsnek added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jun 8, 2018
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, no.

This will break the ecosystem and significantly damage the security and stability of it.

Modules in npm are already fetching files from thirdparty servers upon install, and fetching files from thirdparty servers in start-time would be much worse.

If this gets merged in the current state, packages in npm will end up relying on this.

Currently, updates are installed only when manually requested and could be downgraded. With this, updates would propagate between launches and in some situations there would be no way to roll back (e.g. if the resource holds only the latest version). The users of this is also less likely to follow semver.

If we will make it easy for npm modules to load resources like this, this will end up blowing up much much worse than left-pad did. I don't think it's worth it.

Perhaps, the negative consequences could be minimized if loading-over-http would be allowed only for end programs, not from npm modules (or would be allowed only with an explicit opt-in flag), but I still am not sure that it brings any positive value even in that case.

To be explicit: -1 in the current form.

@joyeecheung
Copy link
Member

joyeecheung commented Jun 8, 2018

Modules in npm are already fetching files from thirdparty servers upon install, and fetching files from thirdparty servers in start-time would be much worse.

Currently, updates are installed only when manually requested and could be downgraded. With this, updates would propagate between launches and in some situations there would be no way to roll back (e.g. if the resource holds only the latest version). The users of this is also less likely to follow semver.

I agree with this, but I don't think that's the particular issue with this PR. What we may need is probably some sort of trusted service that makes rules about the integrity and versioning of these modules (kind of like the npm registry) and we can limit the origins of the modules to a whitelist (possibly configurable by users).
But yes, before we have anything like that in place, this PR alone could introduce a lot of vulnerabilities to the system. I love the idea, but simply having a loader that loads modules over HTTPS is not enough.

(It would be even cooler to be able to load modules over P2P but being cool is not enough either).

@jasnell
Copy link
Member

jasnell commented Jun 9, 2018

Very -1 on this. It completely changes alters the threat model for Node.js in ways that just aren't workable.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also -1 for reasons others have mentioned.

If this were merged and released, it'd be minutes before the first feature request comes in to disable it globally (and rightly so.)

For some historic perspective: pull requests implementing some kind of over-the-wire require() have been filed - and rejected - since the start of the project.

@benjamingr
Copy link
Member

benjamingr commented Jun 9, 2018

Thanks for the effort (cc @nodejs/modules) - we intend (I think) to allow users to have this behaviour with loaders.

This will be possible in Node.js - just not by default in core. If you want this to be brought up please do feel free to open an issue in https://github.com/nodejs/modules

I'm also -1 for the reasons above - thanks a lot for your effort, consider making this a loader that can be released in userland and people can use :)

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above

Copy link
Member

@jdalton jdalton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing the sentiment of @benjamingr. Just ensuring my -1 is explicit.

@viktor-ku
Copy link
Contributor Author

viktor-ku commented Jun 9, 2018

I will open the issue in the modules repo because I believe this loader can benefit some people. And I am fine if this feature won't be enabled in the core by default

Thanks everyone for your feedback

@jimmywarting
Copy link

I think Deno got it right with sandboxing and security.
third party module needs to ask for permission for things like network / file IO.

Deno really solved the modular ecosystem with http imports IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants