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

[WORK IN PROGRESS] experimental dynamic generation of globals #420

Closed
wants to merge 1 commit into from

Conversation

stushurik
Copy link
Contributor

No description provided.

@stushurik
Copy link
Contributor Author

This PR contains a feature that uploads and introspects module from the dependencies list and generates a global name.

I tried it on the next package.json:
image

Copy link
Collaborator

@curran curran left a comment

Choose a reason for hiding this comment

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

It's amazing that this works!

This is a bit "heavyweight" in that

  • it introduces use of eval in VizHub (which is generally dangerous security-wise), and
  • it introduces an additional network request as part of the build step (which may be cached - worth looking into the caching).

I was thinking, what if we track the same data that vizhub-libraries does in the VizHub database? Then we could consult that static data, and only if it's missing information about a certain library, we can use this technique to derive the name of the browser global.

It might even make sense to do this in some sort of more sandboxed environment, like an iFrame or something server-side.

This is very very interesting!

// d3 uses globalThis ¯\_(ツ)_/¯
// possible we will need to collect global context names for other libraries
Function(
'self', 'globalThis',
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis

I think this can only ever be a very limited set of things, including window also.

Suggested change
'self', 'globalThis',
'self', 'globalThis', 'window',

Also check this out https://gist.github.com/coolaj86/1870267

Apparently ["parent", "top", "frames", "self", "window"] are all global aliases.

@curran
Copy link
Collaborator

curran commented Dec 20, 2020

Tracked the D3 globalThis usage back to Rollup: rollup/rollup#3691

Since this is from Rollup, it's probably fairly widespread.

@stushurik
Copy link
Contributor Author

@curran actually its not an eval, its Function constructor, mdn suggest it as safer and more performant alternative

@stushurik
Copy link
Contributor Author

@curran additional request is not a problem, cause it app would make it anyway -- when dependency would be injected as script tag. So it even make some kind of "prefetch" -- when browser would make request caused by script tag, package would be already in cache

@stushurik
Copy link
Contributor Author

@curran and vizhub-libraries might contain stale data

@stushurik
Copy link
Contributor Author

@curran As fast performance improvement I can imagine that we can store parsed data in local storage and maybe do parsing in webworkers

Copy link
Collaborator

@curran curran left a comment

Choose a reason for hiding this comment

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

It is unfortunate that this PR contains a mix of Phase I (make the simplest solution work) and Phase II (experiment with automatically deriving globals).

Could you please split this up into 2 different PRs? One should just have the path implementation, and also the manually configurable global idea (libraries[pkg].global), and the other should contain the experimental stuff. Thanks!

packages/presenters/src/bundle.js Outdated Show resolved Hide resolved
@stushurik stushurik force-pushed the feature/656 branch 2 times, most recently from 9f86889 to a6d70dd Compare January 19, 2021 10:55
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

Successfully merging this pull request may close these issues.

None yet

2 participants