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

Prevent macro from running without babel-marcos #20

Closed
dralletje opened this issue Jul 19, 2017 · 13 comments · Fixed by #24
Closed

Prevent macro from running without babel-marcos #20

dralletje opened this issue Jul 19, 2017 · 13 comments · Fixed by #24

Comments

@dralletje
Copy link
Contributor

I was wondering, as I am using babel-macros in a project now, if there could be a way added so macros could "catch" it when they are being imported in a normal file.

As it seems now, there isn't such thing. The this is unset, if packing with webpack I could check if I am running in node (browser won't), but I also use this on node. Possible would be to check the arguments, but as the mantra of this repo is, I think explicit is better than implicit ;)

I wonder what the best way is though:

  • Setting this seemed good, but won't work with arrow functions
  • Setting a property to check on the macro function itself:
      const macros = require(requirePath)
    + macros.with_babel_macros
      macros({
        references: referencePathsByImportName,
        state,
        babel,
      })
    
    Feels kinda hacky, but again we are putting macros into javascript,
    So who cares about hacky 😅 (This one is actually also possible (cleaner?) with Symbols)
  • Passing a symbol as first argument that can be compared
    import babelMacros from 'babel-macros';
    export default ({ is_babel_macros, references, state }) => {
      if (is_babel_macros !== babelMacros.is_babel_macros) {
        ...
    

Most likely I am missing an option or some perspective, so I'd like to discuss this :)

@kentcdodds
Copy link
Owner

Hmmm... What if you do:

// my.macro.js
const macroLogic = require('./macro-logic')
module.exports = macroLogic({isBabelMacros: true})

// server-thing.js
const macroLogic = require('./macro-logic')
module.exports = macroLogic({isBabelMacros: false})

I never really imagined macro running as anything except a build-time thing. I'm not certain that I understand the use-case... Could you help me understand?

@dralletje
Copy link
Contributor Author

dralletje commented Jul 22, 2017

Hahaha, it's not really a "use-case" I guess, rather a helpful error

But I found myself requiring the macros without having babel-macros installed, and think it would be nice to have an helpful error there :)

@kentcdodds
Copy link
Owner

Gotcha. How do you suppose we could avoid requiring a macro from doing this kind of check manually? My guess is that most wont do it. Is there any way we could just make it work without requiring macro authors from doing it manually?

@dralletje
Copy link
Contributor Author

Yeah, been thinking about that too, but couldn't figure out a way.

Maybe most of them won't do it, but I certainly will. With all the build setup problems, it is too easy to include the macro in a place where it is not being macro-ified, and the error you get by default isn't very helpful either, hahaha.

Then again, I could also check for the state or ref object if you don't want to add this :)

@kentcdodds
Copy link
Owner

Nah, let's add it. Could you do that? I'd probably just change this to this:

macros({
  references: referencePathsByImportName,
  state,
  babel,
  isBabelMacrosCall: true,
})

Then add docs about this property encouraging macro authors to add a check at the start of their macro function and throw a helpful error if that property does not exist.

@kentcdodds
Copy link
Owner

Just talked with @threepointone and he suggested that we change the API for defining a macro slightly:

const createMacro = require('babel-macros/create') // or something like that
module.exports = createMacro(({references, state, babel}) => {
  // do macro stuff
})

And the createMacro function would be something as simple as this:

module.exports = createMacro

function createMacro(macro) {
  return macroWrapper

  function macroWrapper(options) => {
    if (!options.isBabelMacrosCall) {
      // throw error
    }
    return macro(options)
  }
}

This way folks don't have to worry about whether they're running in a compilation process or not. I think this is a great solution and interestingly it's not a breaking change FWIW...

If anyone wants to take this please be my guest :) I'll ping in here when I'm ready to start working on it if nobody else has.

@reznord
Copy link

reznord commented Aug 16, 2017

I'd be happy to take this on!

@kentcdodds
Copy link
Owner

Awesome! Please make sure to update docs! Also, I realized that the authoring docs doesn't have an example. Could you put an example in there? You could base it on one of the examples from the unit tests 👍

@kentcdodds
Copy link
Owner

I'm going to start working on this

@reznord
Copy link

reznord commented Sep 8, 2017

Hey, sorry for the delay on this. I got hung up with some personal stuff. Sorry for not being able to complete it.

@kentcdodds
Copy link
Owner

No worries :) We're all just doing what we have time for 😄

kentcdodds pushed a commit that referenced this issue Sep 8, 2017
Closes #20

This also upgrades all tooling to kcd-scripts
kentcdodds pushed a commit that referenced this issue Sep 8, 2017
This also upgrades all tooling to kcd-scripts

Closes #20
kentcdodds pushed a commit that referenced this issue Sep 8, 2017
This also upgrades all tooling to kcd-scripts

Closes #20
@kentcdodds
Copy link
Owner

Now it's recommended to wrap your macro in createMacro. See the updated docs 👍

@kentcdodds
Copy link
Owner

Now it's recommended to wrap your macro in createMacro. See the updated docs 👍

Not "recommended" ... "required" 🕶

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

Successfully merging a pull request may close this issue.

3 participants