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

Call macro function even if references are empty #65

Closed
wants to merge 2 commits into from

Conversation

eemeli
Copy link

@eemeli eemeli commented May 29, 2018

What:
This PR removes the early exit from applyMacros if no references are found for the import. Effectively, this means that a macro function, if imported, will always be called at least once.

Why:
I'm working on writing a macro for babel-plugin-trace, which repurposes JavaScript's LabeledStatements for logging calls that are by default only compiled in development mode. These statements don't share namespace with anything else, so they're also not returned by the path.scope.getBinding(localName) call.

An alternative solution that could be sufficient for my particular case would be to also check path.scope.getLabel(localName) in the loop, but this could create a conflict as, again, the namespaces for variable bindings and labels are separate. Leaving out the early return also enables an easier API to be used by other macros, as well as this case.

With the current API, this would already be possible (using state.file.path.traverse, and removing the initTrace() call from the AST):

// file.js
import initTrace from 'babel-plugin-trace/macro'
initTrace()

function fun() {
  log: 'This is fun!'
  return 42
}

fun()

// logs when transpiled in dev mode & then run:
//   file:fun: This is fun!

With this PR, the first two lines could be replaced with:

import 'babel-plugin-trace/macro'

I also had to update the cpy dependency to v7 as v6 doesn't work with node v10.

@codecov
Copy link

codecov bot commented May 29, 2018

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #65   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          77     73    -4     
  Branches       18     16    -2     
=====================================
- Hits           77     73    -4
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d8f7a6...8b4a38b. Read the comment docs.

@kentcdodds
Copy link
Owner

Hey @eemeli,

I've thought about this a lot and I think that I don't want to support this. A big part of what makes babel-plugin-macros great is that the transformations are explicit. If we enabled these kinds of macros, then we lose that property. Sorry :-/

@kentcdodds kentcdodds closed this May 29, 2018
@eemeli
Copy link
Author

eemeli commented May 29, 2018

But the macro itself already works with the current release, so is that really a valid reason why not to enable this? Specifically, the state that a macro gets allows for the whole file's node path to be modified, which is what I'm doing with the babel-plugin-trace macro; all I'm asking for here is a less clumsy API for triggering it.

For a live demo of it working, you can install babel-plugin-trace@next and then run the code from the above example.

But I understand that you may consider all of the above to be beside the point, in which case I'd be interested in getting some other means of accessing paths in the scope.labels namespace. Is that something that could be considered?

@kentcdodds
Copy link
Owner

I know that it works as-is and I know that it's clunky. The fact that it's clunky is a feature in my mind. It discourages people to write macros like this one that transform the source code implicitly. I'm sorry. I know this isn't the answer you were hoping for, and I realize the code change is minimal, but the impact on the community would not be minimal and I think it would be a negative president to set. Thank you for understanding.

@eemeli
Copy link
Author

eemeli commented May 29, 2018

Sure, I understand. Disagree, but understand.

So, any thoughts on getting access at the label nodes? Could the macro argument get a labels object that's essentially the same as references, but with results from path.scope.getLabel instead?

@kentcdodds
Copy link
Owner

kentcdodds commented May 29, 2018

That's definitely possible, but it would further encourage implicit transforms. Is there a reason you don't simply use a function call?

@eemeli
Copy link
Author

eemeli commented May 29, 2018

The core idea with babel-plugin-trace is to be able to use label statements as a way to log data during development, and to be able to leave the logging code in the source when compiling for production, because their contents are then removed by the plugin.

Labeled statement make more sense here than normal function calls, because they help underline the fact that we're not dealing with a line of code that's meant to have any impact with the actual app. They're also much easier to distinguish in particular when debugging, and as a core part of JavaScript are already supported by all syntax highlighters. Furthermore they're rarely used in actual code and they have their own namespace, which makes it easier to overload their meaning like this without leaking anything into the app; if you disable the plugin but leave the labeled statements in your code, it'll still work exactly as before. In fact the plugin already internally enforces all logged statements to be non-modifying.

Now, presuming that labels were available to macros, that would enable me to make babel-plugin-trace work with explicit imports being matched to label names, and remove the need for implicit transforms.

@eemeli
Copy link
Author

eemeli commented May 29, 2018

For implementation, labels would probably need to actually be built differently than references for this use case. I could put together a PR if you think it'd have a chance of being accepted?

@kentcdodds
Copy link
Owner

So you're suggesting that people could do:

import {trace} from 'babel-plugin-trace/macro'

trace: 'cool'

I can see what you mean, but as far as language semantics go, the label name cannot be bound to the label, so I just don't see this as promoting the explicitness. It's not something I want to bake into babel-plugin-macros. I'm sorry @eemeli.

@eemeli
Copy link
Author

eemeli commented May 30, 2018

As an update, I caved in and put together dev-console.macro.

@kentcdodds
Copy link
Owner

Thanks for the update. Would you like to add a link to that in the list of macros?

@eemeli
Copy link
Author

eemeli commented May 30, 2018

Sure; feel free to do so. Btw, you might want to check the keywords for this package, as they include babel-plugin-macro rather than babel-plugin-macros. Or is that intentional?

@kentcdodds
Copy link
Owner

Fixed. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants