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

Initialization, scopes and assignments - possible bugs / possible improvements #167

Open
aronmandrella opened this issue Jan 21, 2021 · 10 comments

Comments

@aronmandrella
Copy link

  • babel-plugin-macros version: 3.0.1
  • node version: v14.15.1
  • npm version: 6.14.8

Relevant code or config

// File: test.macro.js

const { createMacro } = require("babel-plugin-macros");

let callbackCounter = 0;

// Counts macro calls and adds comments to every detected macro usage
function macroCallback({ references, state, babel, source, config }) {
  callbackCounter += 1;
  for (const importName in references) {
    for (const path of references[importName]) {
      path.addComment("leading", callbackCounter);
    }
  }
}

module.exports = createMacro(macroCallback);
// File: index.js

macro = 1;
macro;

macro("before require");
const macro = require("./test.macro");
macro("after require");

macro = "Direct assignment is always ignored";
macro.v = "This works";

{
  macro("before require");
  const macro = require("./test.macro");
  macro("after require");
}
macro;
// File: index-after-macro.js

"use strict";

macro = 1;

/*1*/
macro;

/*1*/
macro("before require");

/*1*/
macro("after require");
macro = "Direct assignment is always ignored";

/*1*/
macro.v = "This works";
{
  /*2*/
  macro("before require");

  /*2*/
  macro("after require");
}

/*1*/
macro;

Problem description and suggested solutions:

I can see few issues here:

  • You can access a macro in a scope before initialization. It's a bit unexpected and there are no warnings/errors. It may cause some issues especially if there are some global variables.

  • When you do assignments, direct assignment is completely ignored.
    Even if the macro was imported or required as const there is no error.
    I assume that it has something to do with global variables, but I think
    it would be nice, if a user could handle a scenario like this in a macro.
    Either to just print an error message like 'usage like this is not allowed' or
    to do something else.

  • Babel-plugin-macros allows for scoping (awesome), but the way it's handled right now
    makes it impossible to handle all macro usages within a single function (without some nasty tricks, atleast).
    Maybe there is a way to handle scoping better? Sometimes user may want to do some global post-processing in a file after processing all macros. References could be organised like this for example:

[
  {
    scope: 0, // id
    importName: "default/*/name",
    localName: "importName/as",
    path: PathNode(),
  },
  /* .. */
];

I gives more context about the macro usage.
Of course, localName name currently can be easily checked with Identifier name prop.
The main issue are scopes.

Also two minor things.

  • Syntax like this (similar to require()) is not recognized. Not that I need it, it's just something I noticed. It looks like a minor oversight, but maybe it's intentional.
const macro = import('./test.macro');
  • Syntax like this causes an 'Cannot read property 'name' of undefined' error. Again, just something I noticed.
import * as macro from "./test.macro";
@conartist6
Copy link
Collaborator

conartist6 commented Jan 21, 2021

I see there's an existing bug for import * as macro. I'd be inclined to just throw an error there.

Assignment to the macro failing to trigger anything seems like a major issue to me. That needs its own ticket and I imagine a fix will be reasonably high priority.

You can access a macro in a scope before initialization

I'd argue that's the expected behavior. Macros are compile-time so they will never undergo initialization. It also means there would be never be a reason not to initialize a macro at the top of a scope, or better at the top of a file. After all, you can't do anything clever like use an if conditional to decide which macro to use. I'd say warning when macros are not hoisted above runtime code (either in a block or in a whole file) would be an excellent thing to include in a lint rule.

As far as your suggestion RE scopes, I'd be curious to hear more about your use case. plugin-macros has no desire to stand in for all the use cases that can be supported when writing a full custom babel plugin. I know it's a little fuzzy because in some developers will have the ability to write macros but not plugins.

@aronmandrella
Copy link
Author

aronmandrella commented Jan 23, 2021

As far as your suggestion RE scopes, I'd be curious to hear more about your use case.

I've been experimenting with nested macros, and with stateful macros.
Mostly to learn something about AST on the way, but I also thought they could be useful in some scenarios.
Usage of this kind of macro could look like this for example:

// This returns array of files in a directory
// https://www.npmjs.com/package/files.macro
import files from "files.macro";
// Let say that this filters an array of paths by extension
import filterPaths from "./filter-paths.macro";

// Configuration without a config file (stateful macro)
// Easy to do if constant values are passed like here.
filterPaths.config = {
  endsWith: ".macro.js",
};

// --- Nesting macros ----
// This works but only when "files.macro" is imported first:

// A
const macrosInDir = filterPaths(files("./js"));

// B
const arr = files("./js");
const macrosInDir2 = filterPaths(arr);

It won't work when filterPaths is imported first because filterPaths will be processed first and the argument[0] will be a files("./js") node or a arr Identifier node and not an array node. I didn't find a way to assure the right processing order of macros when there is more than just one macro import statement (even if the same macro is imported) as each import results in a separate call of function passed to createMacro.

Probably for things like this using a preval/preval.macro would be a better choice.

@aronmandrella
Copy link
Author

aronmandrella commented Jan 26, 2021

I'd be curious to hear more about your use case.

So this is the package that I've been working on.
https://www.npmjs.com/package/macroify
A little silly idea, but I think it could be useful for some quick data embedding. I've had the issues that I've described when macros from multiple imports were nested, but when everything is imported in one import statements, after some sorting it works perfectly.

@conartist6
Copy link
Collaborator

Oh my. I'm not sure how I feel about this. It feels to me like the inner platform effect. You're certainly hitting the limits of babel-plugin-macros which wasn't designed for anything this rigorous. But let's step back. The point of macros is to allow users to write code that executes in the transpiler context without needing to alter the build each time. But that is already achieved. What does "macroification" gain a user over just writing a macro?

@aronmandrella
Copy link
Author

aronmandrella commented Jan 26, 2021

What does "macroification" gain a user over just writing a macro?

That he doesn't have to write a macro. It allows a user to use packages that he is familiar with in a new way right of the box. If there is a package that does something (lists files in a directory / validates enviromental variable / synchronously reads a file/html/image/data / and so on), with this wrapper it can be used at build-time without any extra steps. Of couse you can do it with preval.macro too, but with this you can do it with less code and in some alternative ways. Also, because it's all managed by one package, you can safely nest macros when you use only one import. User can also write a custom codegen/preval and many more things in few lines of code even if he don't know anything about babel or AST. That's at least how I see it. I definitly will use it myself for some things, even though I'm not sure how useful it will turnout in the long run. It was mainly a cool project for few nights.

I partially agree with that 'inner platform effect', but I don't think it's that far from things like preval either. It just sorts macros before processing them, and allows some alternative syntaxes.

@conartist6
Copy link
Collaborator

Writing a macro shouldn't be scary, especially since it's just writing cjs/node code. There's tons of resources, documentation, and assistance in helping users with that. The maintainers of this project are invested in making it as painless as possible. Your way would create a new node-embedded-in-plain-js-with-macros environment for which there would be virtually no support.

I guess I don't see why any library needs to be usable as a macro. You can certainly use any library you like in a macro.

And fixing babel templating would make it way, way, way easier for people who don't know AST stuff to do work in babel. I'm specifically referring to this issue, but there would be other advantages to the approach I have in mind, e.g. often a particular code fragment could be interpreted as an expression or a statement and it won't be obvious which is needed until it is known where in the code that fragment belongs. A proper templating solution would understand that when a expression was inserted into a location expecting a statement, it should be wrapped in an ExpressionStatement. That alone should nearly eliminate the current need for plugin authors to express their code using marginally documented AST node constructors.

@aronmandrella
Copy link
Author

Writing a macro shouldn't be scary, especially since it's just writing cjs/node code. (...) The maintainers of this project are invested in making it as painless as possible. (...) I guess I don't see why any library needs to be usable as a macro. You can certainly use any library you like in a macro.

They did a great job. Don't get me wrong. It's not like I think that there is something wrong with current API. I've just noticed, that a lot of macros listed here are basic wrappers around some functions anyway (files.macro, fast-fp.macro, data-uri.macro, paths.macro, ms.macro and more...). I thought that even though babel-plugin-macros is awsome for macros like this one, it requires some unnecessary boilerplate for basic use cases like the ones that I've listed above. So I fiddled around a bit to see if I could find a solution to that. My package doesn't have to be used to turn modules to macros, it can be used to create functional standalone macros too. It just reduces all AST related boilerplate in that case.

Your way would create a new node-embedded-in-plain-js-with-macros environment for which there would be virtually no support.

I know that the detailed READMY may suggests something else, but actually I'm not excepting that many people will start to use it. I've been using NPM for a while now, and I wanted to see how long it takes to create a package with a decent README. I'll use it, if some people will find a use for it I'll be happy, and if no one will, that's fine too. After all, your arguments against an approach like this are very fair and I appreciate them.

@conartist6
Copy link
Collaborator

Yeah I do agree with you that a proliferation of tools just for macro users that mirror builtin functionality will become confusing eventually if it hasn't already. But now that I look closely preval can already do things like require fs APIs and external libraries. The one annoyance about it is that it won't have syntax highlighting inside the preval block, but that is potentially fixable. The syntax highlighting I'm using in VSCode for example highlights GraphQL syntax inside of template strings. If we could convince the syntax maintainers that the name preval is unique enough, maybe preval could get this same treatment?
Screen Shot 2021-01-27 at 10 43 12 AM

Anyway, I really don't want to discourage you because you seem to have the skills and energy to get things done. I more hope that we could convince you to help move the core projects forwards. At the moment I'm the only one actively investing in this code, so I'd love to see more people get involved.

@conartist6
Copy link
Collaborator

Ah, but that still confounds linters and type checkers...

@aronmandrella
Copy link
Author

I really don't want to discourage you

Don't worry, unless more people will start to use it (which is unlikely) I don't plan on developing this package any further anyway as it already fulfills all my needs.

I more hope that we could convince you to help move the core projects forwards.

Open source seems like a good way to learn a lot so I will gladly give it a shot one day, but unfortunately I can't afford to volunteer in such projects right now.

Ah, but that still confounds linters and type checkers...

Yes. That's the issue with things like that, but there are probably ways to make it work too.

Thanks for your opinions and advice. Good luck with you work, too.

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

No branches or pull requests

2 participants