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

Import npm modules #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Import npm modules #17

wants to merge 3 commits into from

Conversation

schempy
Copy link
Collaborator

@schempy schempy commented Jul 1, 2016

Implement importing npm modules as referenced in #8

This is pretty rough but I'm able to import npm modules using https://wzrd.in/. Currently fetching @cycle/core is not working, https://wzrd.in/standalone/@cycle%2Fcore@latest, which is huge but maybe it can be fixed. I could add a condition to only use @cycle/core from the node_modules directory and get all other modules from https://wzrd.in. So far there are no problems importing the other modules.

@Widdershin
Copy link
Owner

Nice work!

I had a play round with the wzrd.in API, and it looks like it works fine if you POST to /multi:

Try this:

$ curl --data '{
  "options": {
    "debug": true
  },
  "dependencies": {
    "@cycle%2Fcore": "latest"
  }
}' https://wzrd.in/multi

This way we can fetch all the modules in one bundle as well. What do you think?

@schempy
Copy link
Collaborator Author

schempy commented Jul 5, 2016

That's awesome! I'll make the changes.

@schempy
Copy link
Collaborator Author

schempy commented Jul 6, 2016

Looks like there is a problem fetching @cycle/core and other modules. No problems fetching @cycle/core by itself.

$ curl --data '{
    "options": {
        "debug": true
    },
    "dependencies": {
        "@cycle%2Fcore": "latest",
        "lodash": "latest"
    }
}' https://wzrd.in/multi

At least I'm able to get @cycle/core doing a POST to /multi

@Widdershin
Copy link
Owner

Widdershin commented Aug 3, 2016

@schempy sorry for the lack of response, this fell off my radar.

When you last updated this, I also received an error attempting to run the above command (install core & lodash).

Now, it seems like it works! Can you check to see if it works for you?

If so, maybe we can move forward?

@schempy
Copy link
Collaborator Author

schempy commented Aug 4, 2016

@Widdershin That's great! It's working for me. I'll make the changes and update the PR.

@schempy
Copy link
Collaborator Author

schempy commented Aug 8, 2016

@Widdershin I included updates that get all npm modules in one request. I'm getting an error that I can't figure out, 'Cannot render into unknown element .app.' Looks like all the modules are getting loaded.

If I override the require function like I do in the following:

const context = {
    error$: error$,
    console: console,
    require: (path) => {
        return moduleCache[path];
    }
};

I get the error. If I don't override the require function like this:

const context = {
    error$: error$,
    console: console,
    require: require
};

Things work. Here's a link to the file.

Any ideas?

Thanks!

@Widdershin
Copy link
Owner

Hi @schempy, sorry for my lack of followup. I'll have a look at this tomorrow.

@Widdershin
Copy link
Owner

I resolved the merge conflicts so I could consider this properly.

I'm getting an error because it looks like wzrd.in has invalid security certificates.

This does raise the question, do we want to rely on the uptime of a third party component?

I think perhaps we should run our own module cache.

}
};

const req = https.request(options, (res) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to make http requests using @cycle/http if possible, to shift side effects out of the main.


vm.runInNewContext(moduleBundle, context);

listener.next({
Copy link
Owner

Choose a reason for hiding this comment

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

Wonder if we could shift this into a driver as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants