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

Reject invalid plugin names #5842

Closed
wants to merge 3 commits into
base: plugin-ordering
from

Conversation

Projects
None yet
5 participants
@sarupbanskota
Contributor

sarupbanskota commented Jun 8, 2017

Q A
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets Progress on #5735 and #5827
License MIT
Doc PR
Dependency Changes

⚠️ This change enforces plugins to have a valid name, that is of type string, and is a valid JS identifier

@sarupbanskota sarupbanskota changed the base branch from 7.0 to plugin-ordering Jun 8, 2017

@@ -2,8 +2,8 @@
export default class Plugin {
constructor(plugin: {}, key?: string) {
if (plugin.name != null && typeof plugin.name !== "string") {
throw new Error("Plugin .name must be a string, null, or undefined");
if (!plugin.name || typeof plugin.name !== "string") {

This comment has been minimized.

@loganfsmyth

loganfsmyth Jun 8, 2017

Member

Hmm, sorry I've been a bit out of the loop. Is there documentation somewhere for why we want to require the name? I'm concerned it'll make it harder for people to upgrade to 7.x

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

yep @loganfsmyth, this was discussed in the meeting on 7 June (altho I wasn't around).

It has to do with simplifying the "intelligent plugin sorting" process.

Benefits

  1. Allows for global topological ordering with the DAG map.
  2. Serves as a known identifying key for the plugin.
  3. Provides opportunity to deduplicate identically configured plugins.

Steps

  1. Deprecate unnamed plugins in Babel 6.
  2. Add deprecation message.
  3. Publish a new patch version with that deprecation.
  4. Write a deprecation guide on how to make this change for your plugins.
  5. Enforce named plugins in Babel 7.

Source: https://docs.google.com/document/d/1zO1QiaAyDxmA1OcmdOKYLFmuSXGSr4S50pE0uQFTVEE/edit#heading=h.bcrawxu2t468.


Also relevant: #5814 (comment)

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

Note that I rebased this branch on top of plugin-ordering :-)

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

Babel 6 stuff is happening on another PR - #5827

@sarupbanskota sarupbanskota force-pushed the sarupbanskota:enforce_plugin_names branch from 0286022 to f0adaec Jun 8, 2017

if (plugin.name != null && typeof plugin.name !== "string") {
throw new Error("Plugin .name must be a string, null, or undefined");
if (!plugin.name || typeof plugin.name !== "string") {
throw new Error("Plugin .name must be a valid JS identifier of type string");

This comment has been minimized.

@jridgewell

jridgewell Jun 8, 2017

Member

"Plugin .name must be a string"?

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

@jridgewell since that constructor is taking in a plugin object with a bunch of properties, I didn't want to make the assumption that the passed in value for name would always be of type string. e.g if it received { name: true } that would still pass the first conditional

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

Happy to just look for name's existence if you think the type check isn't necessary

This comment has been minimized.

@jridgewell

jridgewell Jun 8, 2017

Member

No, I'm commenting on your error message. the "valid JS identifier of type string" is needlessly wordy, just say "needs to be a string"

This comment has been minimized.

@sarupbanskota

sarupbanskota Jun 8, 2017

Contributor

ah, okay - sounds good, thanks for that. I'll update it!

Reject invalid plugin names
Makes progress on #5735 and #5827

@sarupbanskota sarupbanskota force-pushed the sarupbanskota:enforce_plugin_names branch from f0adaec to e1516c6 Jun 8, 2017

@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 8, 2017

I'm guessing the next step will be to name the unnamed core packages?

I wrote a little script:

const fileSystem = require('fs');

const returnFilesWithin = (path) => {
  return fileSystem.readdirSync(path);
}

returnFilesWithin('packages').forEach(package => {
  if (fileSystem.lstatSync(`packages/${package}`).isDirectory()) {
    fileSystem.readFile(`packages/${package}/package.json`, 'utf8', (err, content) => {
      const packageOptions = JSON.parse(content);
      console.log(packageOptions.name);
    });
  }
})

and that printed https://gist.github.com/sarupbanskota/4bf6627eb1501be1b8c14be406164aa4. No empty lines 🤔 What am I missing?

@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 8, 2017

learned from @hzoo that it needs to be within the visitor
e.g in here: https://github.com/babel/babel/blob/7.0/packages/babel-plugin-transform-es2015-computed-properties/src/index.js#L87

visitor: {
  name: "transform-es2015-computed-properties"
  ...
}

i'll think of ways to do it for all core plugin packages


update:

name: "transform-es2015-computed-properties",
visitor: {
  ...
}
@jridgewell

This comment has been minimized.

Member

jridgewell commented Jun 8, 2017

Why does the visitor have a name and not the plugin?

@hzoo

This comment has been minimized.

Member

hzoo commented Jun 8, 2017

yeah that's incorrect - check http://astexplorer.net/ for an example

@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 10, 2017

Just documenting thought process here. Based on two | examples, we wanna do the following:

  1. Iterate over all core babel plugin packages. These are packages within packages/ that match /^babel-plugin/
  2. Store the name as whatever follows babel-plugin-*
  3. codemod the plugin with codemod --require babel-register --plugin plugin.js packages/${package}/src/index.js
  4. plugin.js effectively does the following:
body:
  type: "ExportDefaultDeclaration"
    declaration: 
      type: "FunctionDeclaration"
        body: 
          type: "BlockStatement"
            body:
              type: "ReturnStatement"
                argument: 
                  type: "ObjectExpression"
                    properties: [Array 1]  ← we wanna inject `name` as another prop here

Instead of manually updating filenames on the core packages, I'm doing this programmatically to teach myself to play with the various babel packages available, but as @nathanhammond and @hzoo discussed on Slack, we could use this logic to build a tiny plugin upgrade service or similar

@hzoo

This comment has been minimized.

Member

hzoo commented Jun 10, 2017

Not every plugin has to be formatted in that way but hopefully most are? I would just check for visitor and add the property

https://github.com/babel/kneden/blob/master/src/index.js#L19
https://github.com/tleunen/babel-plugin-module-resolver/blob/master/src/index.js#L25
https://github.com/lodash/babel-plugin-lodash/blob/master/src/index.js#L141

Still don't think the names should be used as capabilities, but if this somehow will give us better error messages ok.

@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 10, 2017

Ah shoot, I made quite a bit of progress and only noticed your comment now @hzoo.

Anyway, here's what I have so far:
Based on the original exploration, I have a plugin that injects a name property with a dummy name atm: https://github.com/sarupbanskota/babel-plugin-add-name-to-plugin/blob/master/src/index.js

export default function({ types: t }) {

  return {
    visitor: {
      ExportDefaultDeclaration(path) {
        let pluginReturn;
        try {
          pluginReturn = path          // "ExportDefaultDeclaration"
            .get('declaration')        // "FunctionDeclaration"
            .get('body')               // "BlockStatement"
            .get('body.0')             // "ReturnStatement"
            .get('argument')           // "ObjectExpression"
            .get('properties.0');      // "ObjectMethod"
        } catch(e) {
          console.log(e);
        } finally {
          const nameKey = t.identifier("name");
          const nameValue = t.stringLiteral("pluginName");
          pluginReturn.insertBefore(t.objectProperty(nameKey, nameValue));
        }
      }
    }
  }
}

Other than the fact that it may not work for every case, please let me know if you see anything if I'm totally in the wrong direction with the way I've written it!

What's pending for me:

  • Make plugin name injection work for different kinds of files (maybe consider @hzoo's visitor comment)
  • Find a way to pass plugin package name into the plugin file for injection
  • Get the plugin out as an npm package
  • Don't inject name if it's already present
@sarupbanskota

What I ran here was the following:

const exec = require('child_process').exec;
const fileSystem = require('fs');

const returnFilesWithin = (path) => {
  return fileSystem.readdirSync(path);
}

returnFilesWithin('packages').forEach(package => {
  const PLUGIN_PACKAGE = /^babel-plugin/;
  if (PLUGIN_PACKAGE.test(package)) {
    if (fileSystem.lstatSync(`packages/${package}/src`).isDirectory()) {
      const cmd = `codemod --require babel-register --plugin plugin.js packages/${package}/src/index.js ${package}`;
      exec(cmd, (error, stdout, stderr) => {
        console.log(package);
      });
    }
  }
});

plugin.js is a plugin I've written locally

@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 16, 2017

Just copying over my message from Slack:

I wrote this plugin https://github.com/babel/babel/pull/5842/files#diff-9db534602dc20174af286fa13dba8b05

and ran this code to run it through babel's core packages https://github.com/babel/babel/pull/5842/files#diff-46ab14e9c7ab9cf574a7e8f116b8d6b0

  1. Does the plugin seem written well? what would you improve?
  2. In some cases, e,g https://github.com/babel/babel/pull/5842/files#diff-2ba358dbf7b25653c724e1acd475a922 seems like both ObjectExpression and ExportDefaultDeclaration got triggered, and there were two modifications to the same file, despite adding a check for it. One way to remove it would be writing 2 plugins, is there a better way?
@sarupbanskota

This comment has been minimized.

Contributor

sarupbanskota commented Jun 23, 2017

I figured out the problem, and will send an update tonight

Name plugins that were missing a name
These changes were done programmatically; the process is
explained at http://bit.ly/2s0r6Oi.

Notes:
babel-plugin-transform-class-properties already has a name.
babel-plugin-transform-regenerator's source seems to be a node_module

@sarupbanskota sarupbanskota force-pushed the sarupbanskota:enforce_plugin_names branch from e4e74e4 to d3360cf Jun 23, 2017

Bump transform-regenerator version
Inject plugin name into original module via
facebook/regenerator#296

@sarupbanskota sarupbanskota force-pushed the sarupbanskota:enforce_plugin_names branch from 16da3d0 to 6db7cc6 Jun 30, 2017

@sarupbanskota sarupbanskota referenced this pull request Jul 6, 2017

Open

[WIP] Plugin Ordering #5735

3 of 7 tasks complete
@loganfsmyth

This comment has been minimized.

Member

loganfsmyth commented Sep 7, 2018

I'm triaging old PR on Babel. Since this has been sitting for ages, I'm going to close it. If you want to rebase this to just add plugin names to our packages, feel free to open a new PR.

@loganfsmyth loganfsmyth closed this Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment