stackvana / hook.io

Open-Source Microservice Hosting Platform
https://hook.io
Other
1.26k stars 117 forks source link

es6 support for required package #204

Closed m0o0scar closed 6 years ago

m0o0scar commented 8 years ago

I choose ECMAScript7 as programming language and write a simple hook with es6 features, it works without any exception:

'use strict';

export default async function (hook) {
  let greeting = `Hello World`;
  hook.res.end(greeting);
}

However when I require another package let pkg = require('some-package'); which use those same es6 features and hit the "Test Code" button, the following error shows up:

/node_modules/some-package/index.js:4
let Args = require('args-js'),
^^^
Unexpected strict mode reserved word
Marak commented 8 years ago

@moscartong - The following code seems to be working:

export default async function (hook) {
  let pkg = require('args-js');
  let greeting = `Hello World`;
  hook.res.end(greeting);
}

https://hook.io/marak/es7-package/source https://hook.io/marak/es7-package

Marak commented 8 years ago

Can you provide me with a created hook containing the code which is erroring?

Marak commented 8 years ago

@moscartong -

I found your service and have reproduced the issue.

I'm not entirely sure why that's happening. I don't use es7 myself and have just recently installed babel to enable es7 for hook.io

Right now all the es7 code is being sent through babel. I guess I should install the latest version of node.js on production and send all es7 code to that binary? Does the latest node support all this syntax?

Summoning @ljharb

ljharb commented 8 years ago

@moscartong the real answer is that args-js has a bug - published modules should NEVER have anything but ES5. That package should be transpiling on prepublish.

@Marak you could run it with babel-node instead of node, but a) i'm still not sure that would transpile things in node_modules, and b) we shouldn't be enabling people to use hostile packages that don't transpile prior to publishing.

Marak commented 8 years ago

@ljharb

I think I understand. I'm good with not changing anything, as it will take less time.

Is this a recurring problem in es7 / babel land? I have no idea, as I am not currently using any of these tool chains anywhere else but here.

ljharb commented 8 years ago

It's not a common problem - most packages that fail to transpile prior to publishing get widely shamed and shunned by the community pretty quickly.

m0o0scar commented 8 years ago

@Ijharb Thanks for identifing the problem and the suggestions. Indeed some dependencies are written in es6 and did not babelify before publish. I already updated those dependencies to compile to es5 before publish to npm. But it seems that the version that hook.io is using is still the old ones. How do I update npm packages in hook.io?

Marak commented 8 years ago

@moscartong -

I have updated wps-htmlify to latest version.

http://hook.io/moscartong/htmlify-delete is now working?

Marak commented 8 years ago

Currently it seems we have no way to update npm package versions. Awesome.

I think we are going to move to an approach where we have every single npm module in the entire registry installed by default including all versions. We could then allow users to require specific versions of any package without worrying about installing it.

ljharb commented 8 years ago

I think that's great :+1: at that point you're just building a transparent caching layer over the real registry, which is a good thing.

Be aware that data regarding unpublishes, deprecation messages, and updated dist-tags will continually need to be re-fetched to avoid problems.

m0o0scar commented 8 years ago

@Marak Yes it is working now :) Thanks man! It there any way that I could trigger the npm package update from hook.io's microservice management web page manually ? We are working with some frequently-changing packages and may require version update very often.

Marak commented 8 years ago

@ljharb - Do you know any tooling which could do this for npm without much configuration?

I remember seeing many implementations of this. Unsure what the current state of the eco-system is.

ljharb commented 8 years ago

Your best bet is probably https://www.npmjs.com/package/sinopia

m0o0scar commented 8 years ago

@Marak I use a npm package named "npm-check-updates" to check and install latest version for my project's dependencies automatically. Maybe you could take a look?

Marak commented 8 years ago

Unfortunately, It's not that simple. I can add an extra API endpoint quickly that would allow for you to manually install specific updated versions of packages into your chroot worker environment.

I might not do this at all in favor of implementing a better API where you could require any version of a module using a format like require('colors@1.1.2');

@moscartong - Would you be able to upgrade to a paid account? If you signup for our basic plan I will make a priority update to give you this functionality today.

ljharb commented 8 years ago

@Marak i think that would be a much worse API because it's different from normal node. What about allowing a second file named package.json in the main gist, that works like a normal package.json?

Marak commented 8 years ago

@ljharb - You have a good point.

We have some package.json integration already, but it's mostly a placeholder. see: http://hook.io/marak/echo/package and also https://github.com/bigcompany/hooks

The issue becomes how do we support multiple versions of same package inside the same worker environment. The packages need to be cached for every execution. Cached as in, already installed in the node_modules/ directory for that environment.

Having a separate worker environment per hook service would be good, but is goes against our current scaling strategy ( which is vertical, not horizontal ).

A package.json manifest is good to have for describing a service, and we will maintain keeping dependencies populated with the services deps. What I'd like to avoid is making the package manifest mandatory. It's way easier to just require a module and not have to think about it.

@ljharb -

What is the real downside of implementing such an API like require('colors@1.1.2')?

require('colors') would always use the latest installed version. require('colors@1.1.x') would respect semantic versioning. I'm trying to remember if this conversation came up in Node Core or NPM discussions before.

ljharb commented 8 years ago

I'd say requiring package.json for "not the latest" makes the most sense to me. The issue with an API like that is more that it decreases further the copy-pasteability of the code, since normal require doesn't deal with versions in any way.

Marak commented 8 years ago

@ljharb -

I'm not really sure what we could do then. The workers share the same environment for npm deps. If we don't mess with require in the spawned node vm we won't be able to support loading multiple versions of dependencies at all.

To implement package.json support, we'd still need to have multiple versions of a dependency living in the same system. I don't think npm allows for that at all. ied supports multiple versions of deps.

ljharb commented 8 years ago

npm allows for that if node_modules dirs are nested - and you could use symlinks to avoid having duplicate copies of deps on the filesystem (which is basically what ied does).

In other words, each hook would be in its own dir and have its own node_modules dir

Marak commented 8 years ago

That might be the best approach. We need to start isolating the service environments better anyway.

This also seems like a requirement for https://github.com/bigcompany/hook.io/issues/142

rockymadden commented 8 years ago

@Marak I like the approach you suggested above (e.g. require('colors@1.1.2')), but perhaps with tweaks. You guys could override require under the hood and allow for curried interactions, like Tonic does:

Allowing for standard usage in which it might use the global default, or when used in it's curried form use the version specified.

Marak commented 8 years ago

@rockymadden -

I'm not sure how tonic dev does it, but I think our general plan is to override require.

rockymadden commented 8 years ago

@Marak I imagine the same override idea with something like multimethod dispatch. :+1: Great stuff

Marak commented 8 years ago

@rockymadden - Any sample code or examples would be welcomed.