nuvolo / sincronia

Modern build pipeline for ServiceNow
GNU General Public License v3.0
40 stars 19 forks source link

Babel Preset: Avoid transpiling to __proto__ where possible #137

Closed chaorace closed 3 years ago

chaorace commented 4 years ago

Hello! I noticed this project's Babel preset on npm and wanted to share my own personal approach to the __proto__ issue affecting scoped applications.

While it is not possible to correctly transpile, say, a random __proto__ call to Scope-friendly code, it is absolutely possible to rewrite the babel helpers to completely avoid using __proto__ in the first place. imo, this resolves 90% of the scenarios where you would be otherwise forced to stub a reference to __proto__.

The two areas where Babel's helpers use __proto__ are in the setPrototypeOf and getPrototypeOf methods. These can be refactored to avoid using __proto__ at all, like so:

'getPrototypeOf': function (o) {
    var O = Object(o);
    if (typeof O.constructor == 'function' && O instanceof O.constructor) {
        return O.constructor.prototype;
    }
    return O instanceof Object ? Object.prototype : null;
},
'setPrototypeOf': function (o, proto) {
    function Chain(o) {
        for (var k in o) {
            if(Object.prototype.hasOwnProperty.call(o, k)){
                this[k] = o[k];
            }
        }
        Chain.prototype = null;
    }
    Chain.prototype = proto;
    return new Chain(O);
},

There is a caveat, however! You must also rewrite other babel helpers that utilize setPrototypeOf to manually reassign the target variable to the function result. Doing it this way specifically is important, due to... engine weirdness, I think? (I would be happy to be wrong about that, though!)

For example, this helper:

'construct': function (Parent, args, Klass) {
    var a = [null];
    a.push.apply(a, args);
    var Constructor = Function.bind.apply(Parent, a);
    var instance = new Constructor();
    if (Klass) exports.setPrototypeOf(instance, Klass.prototype);
    return instance;
},

Would need to be rewritten to perform reassignment on the instance reference, like this:

if (Klass) instance = exports.setPrototypeOf(instance, Klass.prototype);

For the record, the main helpers that utilize setPrototypeOf seem to be inherits, construct, and wrapNativeSuper. This holds true when using the following babel plugin list (in order of execution):

        ['@babel/plugin-external-helpers'],
        ['@babel/plugin-transform-for-of', { loose: true }],
        ['@babel/plugin-proposal-nullish-coalescing-operator', { loose: true }],
        ['@babel/plugin-proposal-optional-catch-binding'],
        ['@babel/plugin-proposal-optional-chaining', { loose: true }],
        ['@babel/plugin-transform-member-expression-literals'],
        ['@babel/plugin-transform-property-literals'],
        ['@babel/plugin-transform-arrow-functions'],
        ['@babel/plugin-transform-block-scoped-functions'],
        ['@babel/plugin-transform-block-scoping'],
        ['@babel/plugin-proposal-class-properties', { loose: true }],
        ['@babel/plugin-transform-classes', { loose: true }],
        ['@babel/plugin-transform-computed-properties', { loose: true }],
        ['@babel/plugin-transform-destructuring', { loose: true }],
        ['@babel/plugin-transform-literals'],
        ['@babel/plugin-transform-parameters', { loose: true }],
        ['@babel/plugin-transform-shorthand-properties'],
        ['@babel/plugin-transform-spread', { loose: true }],
        ['@babel/plugin-proposal-object-rest-spread', { loose: true }],
        ['@babel/plugin-transform-template-literals', { loose: true }],
        ['@babel/plugin-transform-exponentiation-operator'],

See this gist for the full babelHelpers that I am currently using.

bbarber9 commented 4 years ago

@chaorace Thanks for submitting such thorough feedback! I'm not a babel expert so it's fun to hear more opinions on our approach. Could you elaborate on the advantages of your approach over the one we currently use?

chaorace commented 4 years ago

@bbarber9 The current implementation essentially sandpits __proto__, as far as I can tell. Since Babel makes use of __proto__ in setPrototypeOf and getPrototypeOf, this causes any transpiled output using those methods to behave in unexpected ways (class declarations, for example).

By replacing the default babel polyfills with a __proto__-free implementation, you can effectively section off this undesirable sandpitting to only affect direct usages of __proto__. Babel supports replacing their default "helper" polyfills (_wrapNativeSuper, for example) via the @babel/plugin-external-helpers package. If you load it, Babel will stop inlining helpers in generated files and will instead point to a global babelHelpers object, where it expects all needed helpers to be defined.

Doing this not only allows you to redefine individual polyfills to suit SNOW's weird environment, it also considerably reduces the filesize of each generated file, since the helpers are no longer inlined inside each file.

Because Script Includes are so flexible, you can actually just upload that babelHelpers file I linked previously to your instance as a global script include. It is quite handy for making the generated code more readable. On that note: I highly recommend enabling the retainLines babel output option. It makes stack traces waaaay easier to decipher while also providing more convenient locations to attach debugger breakpoints).

chaorace commented 4 years ago

As for the plugins list I previously provided:

I would recommend using that list of plugins over using the default babel env preset, which is what you appear to currently be doing. The env preset is meant to target modern(ish) browsers, as opposed to the archaic version of Rhino that SNOW is using. Those plugins, loaded in that order, are what I've found to work most consistently with the minimal possible overhead.

If you use a different preset or roll your own plugins, you will need to modify your babelHelpers to include any additional helpers that may be expected by those plugins/presets, since the previously linked file was built according to my plugin list.

bbarber9 commented 4 years ago

Ok so let me know if I'm getting this wrong:

I see two potential enhancements here

  1. Remove the babel-env setup in the current servicenow preset and replace them with a curated list of plugins that produce minimal overhead on the Rhino Engine. Also enable the retainLines for better debugging.

  2. Make a potentially much better version of the preset by rolling our own implementations of various babel transforms to make them work as expected and replace __proto__

Number 1 sounds like a great idea and I'd happily accept a PR on that given that it doesn't introduce any breaking changes.

Number 2 sounds exremely interesting to me especially b/c I could see us building some SN-compatible polyfills to add even more functionality that our current preset doesn't provide. However I'm hesitant to include it in sincronia proper right now b/c I don't think I currently have the capacity to maintain such an implementation. If you do build something like this, I would be more than excited to try it out and link it on the sincronia docs.

Thanks again for your very thorough explanation and let me know if I'm misunderstanding anything here.

chaorace commented 4 years ago

That is correct. Though it's important to note that babelHelpers is an all-or-nothing deal. You have to provide all polyfills to make it work. Technically, there is a whitelist property you can put into the plugin config parameter, but it's undocumented, so caveat emptor.

Typically, you'll want to decide on your plugin list before you generate a babelHelpers file. If you do it the other way around, you'll be stuck patching in new helpers as you go.

chaorace commented 4 years ago

I might submit a PR, but... I don't actually use Sincronia. There's no way for me to actually test any changes I do submit.

For now, I'm quite happy rolling my own hacked together bundler (see here, if you're interested in exchanging some ideas), since I need something that integrates with the Service Now VS Code extension. I may be interested in contributing the changes necessary to make the file structure consistent with what the VS Code extension uses so that the two tools are compatible, but that's obviously a pretty major change and I don't know if such compatibility is something your team would consider worthwhile.

For reference, this is the table -> folder mapping that the VSCode extension utilizes. If you want to dig deeper, I might suggest unpacking the VSCode extension. They're shipping release artefacts with the full TS source still zipped in for some reason :man_shrugging:

bbarber9 commented 4 years ago

Awesome glad I got a good understanding!

That's awesome you have your own stuff! It's nice to see other people getting into ServiceNow bundling! This ecosystem needs it so bad. I might have a look-see and see what's going on over there in your neck of the woods. Happy to share experiences anytime.

Out of curiosity, what keeps you tied to the SN VS Code extension?

chaorace commented 4 years ago

Very deep integrations all around, they're really packing in the features to the point where it's irreplaceable. To list off the major upsides:

chaorace commented 4 years ago

Oh yeah! I think this is worth mentioning: those source files in the VS Code extension include the full d.ts API bindings. I notice you guys have been rolling your own binding files, so it might be worth cross referencing to find any holes in your own documentation.

I'd love if a team did some clean-room reverse engineering on those bindings so that the community has decent bindings that can be safely distributed.

One last question! This is entirely unrelated, but you guys don't have discussions enabled, so I'm not sure where to ask: How powerful is your plugin system? If I wanted to, could I implement my desired directory layout as a plugin or is the current structure fairly baked in?

bbarber9 commented 4 years ago

Yeah I saw those bindings when I tried out the VS Code extension a while back. The impression I got was they built the types as a way to provide intellisense to users writing JS instead of providing a rich typescript experience. The types we have are generated from SN's docs site and then we put some additional polish onto what we get from their site. For example, GlideRecord's type is generic, allowing us to supply a type for the table so we can get intellisense on the fields and references etc.

The plugin system is pretty powerful, but it's focused on the transformation of files instead of affecting the core functionality of sincronia. The file structure is used as a way to derive where to put the code after transformation (table>record>field) so right now it's baked in. The advantage of this is we can support just about any table, even custom ones that you build and want to code in. However, I understand the desire to have more readable folder + file names. I think it would be possible to build some kind of alias setup like you posted with SN's solution. I just haven't gotten around to it yet :)

chaorace commented 4 years ago

You may be surprised to hear that the VS Code extension also supports adding arbitrary tables and fields. It's driven by the "Add Custom File Types" command. As the extension is today, I can work with any field from any table. The mapping format is stored as a JSON at the top level of a given project and is something that could work pretty seamlessly with external tooling.

In any case, I'll go ahead and create a new issue to track that particular request. I would be thrilled if it's ever added in!

bbarber9 commented 3 years ago

Closed since we opened a new issue