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

Handling feature detections #2273

Closed
guybedford opened this issue Jun 14, 2018 · 8 comments · Fixed by #2892
Closed

Handling feature detections #2273

guybedford opened this issue Jun 14, 2018 · 8 comments · Fixed by #2892

Comments

@guybedford
Copy link
Contributor

Some ideas:

  1. Deopt any literal value or return value interpretations when inside a try {} block, or even just deopt try blocks entirely.
  2. On the "literal members" definitions, include a special property "featureDetection" whether that member might want to be feature detected. In those cases, we add unknown uncertainty to the feature detection patterns (typeof X, Object.create === undefined, etc)
@lukastaegert
Copy link
Member

As for 1., we could talk about this but only if we add a flag to the tree-shake section to enable tree-shaking inside try blocks again.

Thinking about 2., no, this is IMO not the place to put this. Feature detection will usually happen on globals, cf. your example with Object.create. We do not need to do feature detection on locals as we know everything about them.

So the simplest approach is the one that already works: If we make sure that getLiteralValueAtPath always returns UNKNOWN_VALUE for globals, all feature detection examples that rely on typeof or comparisons with literals should work out of the box.

I really do not like the idea of a featureDetection flag if it can be avoided easily.

@lukastaegert
Copy link
Member

Also note that with the current architecture, it is very difficult (and probably not desirable anyway) to prevent tree-shaking inside functions called from try-blocks.

@lukastaegert
Copy link
Member

As for 1 again, as I think feature detection is always related to globals, maybe this could be a nice approach:

  • We add a flag to the execution path options if we are in a try-block (which is reset in nested function calls as once those functions are included, the flag will be forgotten anyway); BTW these flags were the reason execution path options got their name
  • Inside a try block
    • accessing any globals is a side-effect
    • calling or instantiating globals or members of globals is a side-effect

This is of course not perfect due to the restrictions on nested function calls but could solve many issues.

@lukastaegert
Copy link
Member

So in short I should be possible to put nearly everything into GlobalVariable

@guybedford
Copy link
Contributor Author

Throwing further ideas out here -

  1. It might be useful to allow defining globals as an option (eg I know my target environment will have certain globals)
  2. "env target" information like babel env preset could be useful to say do a modern browser build that optimizes out unnecessary feature detections

Perhaps if there was a nice way to do (1), (2) could actually be entirely handled in the plugin sphere.

@lukastaegert
Copy link
Member

The more I think about it, the more I begin to like the idea of defining globals via an option. This may not be an option for everyone but it may be very useful for people fine-tuning libraries and of course plugin developers.

If we pair this idea with reworking the handling of globals and make this powerful enough, very cool things could become possible:

  • There are still some libraries around that expose their interface via globals. With such an option, a rollup plugin for this library could precisely define which properties can be accessed safely and which methods have side-effects and which do not.
  • This idea could even be extended to handle externals as well
  • And of course, people could fine-tune their code with regard to globals and externals
  • On the other hand, such an option could be used to "deoptimize" standard JavaScript functions for whatever reason people could want that

@WebReflection
Copy link

WebReflection commented Nov 22, 2018

I've just found out how broken are features detection with rollup.

The most basic example is this one:

try { new Event('!'); }
catch (o_O) {
  Event = function () {};
}

export default Event;

All I want to know is if there is a global Event and if that's usable as constructor.

The exported result ?

try {}
catch (o_O) {
  Event = function () {};
}

A completely useless, because empty, try and goodbye feature detection, welcome slow fallbacks for everyone.

This happens the same with new Map or new WeakSet and all others.

Is there any estimation on how this will be fixed/trustable?

@lukastaegert
Copy link
Member

I actually went for the try-statement deoptimization in #2892. No feature-flag yet but some logic to limit its infectious nature.

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