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

feat(transform-runtime): add more type hints for instance methods #10068

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devinus
Copy link

@devinus devinus commented Jun 9, 2019

Q                       A
Fixed Issues? No
Patch: Bug Fix? Potentially
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

Continues the work from #9754 and adds more hints to the corejs3 definition type hints.

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 9, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10920/

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10910/

@devinus devinus force-pushed the feature/more-instance-method-hints branch from f21a95c to f0db990 Compare June 9, 2019 03:21
@devinus devinus force-pushed the feature/more-instance-method-hints branch from f0db990 to 7764056 Compare June 9, 2019 05:09
@devinus devinus force-pushed the feature/more-instance-method-hints branch from 7764056 to 922ce97 Compare June 9, 2019 05:27
flat: { stable: true, path: "flat", types: ["array"] },
forEach: { stable: true, path: "for-each", types: ["array"] },
includes: { stable: true, path: "includes", types: ["array", "string"] },
indexOf: { stable: true, path: "index-of", types: ["array"] },
Copy link
Member

Choose a reason for hiding this comment

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

❗ I think that this is the only actually needed line in this PR, since we don't need to load the polyfill for "".indexOf.
All the other additions only prevent us from adding polyfills for code that will throw at runtime.

Copy link
Author

Choose a reason for hiding this comment

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

Doesn't it also prevent polyfilling for literals that may have had their prototypes monkey-patched? Which is really dumb but also possible.

Copy link
Member

Choose a reason for hiding this comment

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

Seems, for better effect, we should extend typeAnnotationToString for work with more classes / types.

flags: { stable: true, path: "flags", types: ["regexp"] },
flatMap: { stable: true, path: "flat-map", types: ["array"] },
flat: { stable: true, path: "flat", types: ["array"] },
forEach: { stable: true, path: "for-each", types: ["array"] },
Copy link
Member

Choose a reason for hiding this comment

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

This is also for map/set, but I think that Babel handles them as "unknown" and thus adds the polyfill anyway?

Copy link
Author

Choose a reason for hiding this comment

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

I think that Babel handles them as "unknown" and thus adds the polyfill anyway?

Correct.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test? I don't want to introduce bugs in case we decide to optimize the logic also for map/set other than array/etc.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with Map and Set that even if we will add naive detection of them to typeAnnotationToString to prevent adding unnecessary .forEach polyfill, Map anyway could be transpiled to something like _Map by runtime transform.

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.

None yet

4 participants