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

Support options to babel-macro #26

Closed
vinhlh opened this issue Sep 8, 2017 · 10 comments
Closed

Support options to babel-macro #26

vinhlh opened this issue Sep 8, 2017 · 10 comments

Comments

@vinhlh
Copy link

vinhlh commented Sep 8, 2017

Problem

import t from 'tagged-translations/macro';

const name = 'Vinh Le';

t`Hello ${name}`;

This is my use case that I need to have some configurations. It could be the location of the translation file.

The current version has no way to do this.
We might need to configure these options globally.

@kentcdodds
Copy link
Owner

Hey @vinhlh! 👋

I can think of a few ways to do this.

Probably the best is to use cosmiconfig and in your plugin you search for the relevant config based on the file (which you can find from the state object. I think that would be pretty straightforward.

Alternatively, you could do:

import t from 'tagged-translations/macro';

t.config({
  // some literal
})

const name = 'Vinh Le';

t`Hello ${name}`;

And you could use it in your macro as well. That's what I had in mind for some sort of config solution.

I don't think that I want to expose a way to configure individual macros via the babel-macros config in .babelrc because that could get unwieldy. I think my suggestion is a pretty good solution. So I'll go ahead and close it. But feel free to continue dialog and if we decide that babel-macros could do something better to support this use case then we can reopen the issue.

Thanks a bunch!

Oh, and don't forget to upgrade to 1.0.0!

@davidtheclark
Copy link

davidtheclark commented Sep 17, 2017

@kentcdodds I respect your decision here, but just wanted to throw in a counterargument for consideration.

If the goal of babel-macros is to make macro authoring and usage easier and more standardized, configuration seems to fall into that scope. The proposed solutions introduce some challenges that authors and users would have to confront:

  • Every macro will need to handle its own configuration loading. This reduces the value of babel-macros' standardization. This also makes macro authoring more difficult.
  • Passing object literals to functions only works for macros that are functions.
  • There's a variety of ways an author would need to validate an options object passed to a function, and those ways would conflict with user needs. For example, if someone used this translation macro in a variety of places and had to configure it in every place, they'd want to store the config in a variable somewhere and reuse it. But that usage would be difficult or maybe impossible for the macro author to interpret, right?

Given those problems, I wonder if configuring macros with the babel-macros config in .babelrc would actually turn out less unwieldy than the alternatives.

(Another possibility would be to introduce a babel-macros configuration, loaded with cosmiconfig — but I'm not sure that provides benefits over the .babelrc approach.)

@kentcdodds
Copy link
Owner

Hmmm... I guess I'm having trouble because I can only think of a handful of situations where configuration is needed... Could you give me some real examples where configuration is necessary?

I definitely want to avoid config via babelrc because then it won't work with CRA

@davidtheclark
Copy link

I can only think of a handful of situations where configuration is needed

@kentcdodds: Interesting, because I'm having trouble thinking of macros that I wouldn't want configuration for. I guess the preval and codegen macros you've written don't need config. Here are some ideas for macros that I think would benefit from configuration:

@kentcdodds
Copy link
Owner

Thanks @davidtheclark! I think that I'm warming up to it. I think that @threepointone also has some thoughts as well.

@kentcdodds
Copy link
Owner

I'm going to reopen this... We'll probably do something to make config easier.

@kentcdodds kentcdodds reopened this Oct 12, 2017
@kentcdodds
Copy link
Owner

I think that I want to go with the cosmiconfig approach. To address @davidtheclark's comment:

Another possibility would be to introduce a babel-macros configuration, loaded with cosmiconfig — but I'm not sure that provides benefits over the .babelrc approach.

The biggest problem with the .babelrc approach is that it requires that you have the ability to edit the babel configuration. The original purpose of this project was so we could use babel plugins in a tool that abstracts away the babel configuration (specifically create-react-app). If some macros were to require you configure them in .babelrc then we'd be no better off.

So for that reason, I think that cosmiconfig is a good way to go about doing this. I'm going to implement it as an experimental feature. This means that the API could have breaking changes without warning/semver bumps until I feel comfortable with what we've done.

My first impression is to do this:

const {create: createMacro} = require('babel-macros')
const configName = 'taggedTranslations'
module.exports = createMacro(myMacro, {configName})
function myMacro({references, state, babel, config}) {
  // config would be taggedTranslations portion of the config as loaded from `cosmiconfig`
}

The config would be called babel-macros and could be configured in the following places:

  • .babel-macrosrc
  • .babel-macrosrc.json
  • .babel-macrosrc.yaml
  • .babel-macrosrc.yml
  • .babel-macrosrc.js
  • babel-macros.config.js
  • babelMacros in package.json

With the way that cosmiconfig works, I think that this would be a pretty good way to handle config for babel-macros.

I'm going to start working on this until my wife gets home or the kids wake up. Please ping me here if you have issues, ideas, or comments.

cc @threepointone

@kentcdodds
Copy link
Owner

Got a WIP: #36

@kentcdodds
Copy link
Owner

Please review :)

kentcdodds pushed a commit that referenced this issue Oct 15, 2017
@kentcdodds
Copy link
Owner

Ok, the PR is ready. I want at least one review before I merge it.

kentcdodds pushed a commit that referenced this issue Oct 15, 2017
kentcdodds pushed a commit that referenced this issue Oct 15, 2017
kentcdodds pushed a commit that referenced this issue Oct 15, 2017
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

3 participants