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

Implement "Records and Tuples" transform #12145

Merged
merged 9 commits into from Feb 21, 2021

Conversation

rickbutton
Copy link
Contributor

@rickbutton rickbutton commented Oct 6, 2020

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 👍
Tests Added + Pass? 👍
Documentation PR Link
Any Dependency Changes?
License Apache-2.0

This PR adds a transform for the Stage 2 Record and Tuple syntax. By default, the transform will look for Record and Tuple in the global, but will optionally look up the names in a module, if either the importPolyfill option is set to true, or a polyfill module name is specified via polyfillModuleName.

We have an experimental polyfill available at https://github.com/bloomberg/record-tuple-polyfill, which I intend to publish as the @bloomberg/record-tuple-polyfill package soon.

We originally open sourced this work under the Apache-2.0 license, and have included said license in the plugin's subdirectory.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 6, 2020

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

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Record Tuple labels Oct 6, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 6, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cd25b57:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks Rick! I planned to port the plugin to this repo but never actually found the time to do so. I really appreciate you doing this!

Two things for the Babel team:

  1. I have no idea about how licenses work and what it compatible with what. We already have other exceptions in our codebase (you can check them in .gitignore), but they are all MIT.

  2. Currently this plugin transforms records and tuples syntax to a call to the builtin, and loads the polyfill for such builtin function.
    This makes this plugin "self-contained", however we might prefer to do something similar to regenerator-runtime (or to other transforms relying on builtins): just inject a reference to the global, and then let our users load the polyfill themselves*. This would make the plugin polyfill-agnostic (however, only one polyfill currently exists), but it requires user to explicitly specify the polyfill.

    In this case, the plugin could be simplified to something like this:

    export default declare(({ template, types: t }) => {
        api.assertVersion(7);
    
        return {
            name: "@babel/plugin-proposal-record-and-tuple",
            inherits: syntaxRecordAndTuple,
            RecordExpression(path) {
                path.node.type = "ObjectExpression";
                path.replaceWith(template.expression.ast`Record(${path.node})`);
            },
            TupleExpression(path) {
                path.replaceWith(
                    t.callExpression(t.identifier("Tuple"), path.node.elements)
                );
            }
        };
    });

* "Themselves" could be using a new polyfill provider in https://github.com/babel/babel-polyfills, or just core-js/es-shims when they implement Record/Tuple support.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.13.0 milestone Oct 15, 2020
@tjpalmer
Copy link

I'd love to include this in a YouTube video I want to make this weekend. (Channel with 10K subscribers.) Would be good timing with C# 9 and its records just out. Oh well.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

One minor comment, but otherwise this looks good.

@rickbutton
Copy link
Contributor Author

apologies for the delay, addressed the comment and resolved the merge conflicts, should be good to go!

@nicolo-ribaudo
Copy link
Member

Thanks! And don't worry for the delay, we haven't scheduled 7.13 yet anyway.

@rickbutton rickbutton force-pushed the rb/record-and-tuple-transform branch 2 times, most recently from 7101e79 to 75caf98 Compare December 16, 2020 16:58
@rickbutton
Copy link
Contributor Author

thanks for the review @JLHwung , I think I've addressed your comments.

@JLHwung
Copy link
Contributor

JLHwung commented Dec 16, 2020

@rickbutton Thanks! Can you re-run yarn and commit the modified lockfile?

@rickbutton
Copy link
Contributor Author

@JLHwung done

This was referenced Mar 14, 2021
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 24, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release Spec: Record Tuple
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants