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

Imports that rely on "this" being window don't work #1

Closed
gzuidhof opened this issue Aug 4, 2020 · 5 comments
Closed

Imports that rely on "this" being window don't work #1

gzuidhof opened this issue Aug 4, 2020 · 5 comments

Comments

@gzuidhof
Copy link
Owner

gzuidhof commented Aug 4, 2020

An example is rxjs, when importing:

await import("https://cdn.jsdelivr.net/npm/rxjs@6.6.2/bundles/rxjs.umd.js");
TypeError: global is undefined @https://cdn.jsdelivr.net/npm/rxjs@6.6.2/bundles/rxjs.umd.js:416:1

The code around that line is:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
    typeof define === 'function' && define.amd ? define('rxjs', ['exports'], factory) :
    (factory((global.rxjs = {})));
}(this, (function (exports) { 'use strict';
@FredKSchott
Copy link

You can try out Skypack CDN instead, every npm package is served as valid ESM for import.

// works!
await import('https://cdn.skypack.dev/rxjs')

@gzuidhof
Copy link
Owner Author

gzuidhof commented Aug 6, 2020

Perfect, I think Pika and Skypack are the perfect companions to Starboard.

I'm thinking about exposing a global helper function, something along the lines of:

async function skypackImport(packageName: string, opts: {min: boolean} = {min: true}) {
  return await import(`https://cdn.skypack.dev/${packageName}${opts.min ? "?min" : ""}`;
}

As for the value of this in cells, a cell's code gets wrapped like this:

Original cell code

console.log("Hello!");
await fetch("https://example.com");

Wrapped cell code

(async () => {console.log("Hello!");
return {returnValue: (await fetch("https://example.com"))};
})()

We could use Function.call() instead to pass in window as this.. but other than imports misbehaving I'm not sure whether it's worth supporting "this". It's much better if people use window or globalThis explicitly.
Do you have any thoughts on this?

I suppose it is trivial to support, so maybe it's just worth doing so people not using skypack don't run into this issue..

@gzuidhof
Copy link
Owner Author

gzuidhof commented Aug 6, 2020

Scratch that trivial Function.call solution, this import doesn't work either:

(async () => {return {returnValue: (await import("https://cdn.jsdelivr.net/npm/rxjs@6.6.2/bundles/rxjs.umd.js"))};
}).call(window) 

To my surprise, using console.log(this) already points to Window without the changes to transpilation above, even in strict mode, maybe that's an eval thing.

I suppose within the dynamic imported script this can not be manipulated.

@FredKSchott
Copy link

That's awesome, and exactly the kind of workflow Skypack is built for!

Users are so used to being able to just import packages by name, you could go the extra step and have Skypack power import('rxjs') directly. You'd just need to do a simple rewrite behind the scenes when you go to run the user's code: from import('rxjs') -> import('https://cdn.skypack.dev/rxjs')

@gzuidhof
Copy link
Owner Author

gzuidhof commented Aug 6, 2020

Even though I think it would make usability a lot better, I'm hesitant to introduce that change.

The logic whether to rewrite or not is difficult to get right in every situation. If somebody embeds a notebook on their own website and wants to import a script "myscript.js" that is hosted on their own server, that shouldn't be rewritten. Some npm packages have .js in their name making it even harder to distinguish.

I think it makes sense for requiring an explicit opt-in to the magic for now by exposing a function (maybe in the future it could be behind a toggle).

@gzuidhof gzuidhof closed this as completed Aug 8, 2020
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

2 participants