Allow macro config to come from plugin options in babel config. #113

conartist6 · 2019-05-25T23:18:04Z

Fixes #112

I'm missing test coverage, but I'm also not familiar with the way this test environment is set up. Some tips on how to test this well would be appreciated.

conartist6 · 2019-05-25T23:21:35Z

other/docs/author.md

-* [Writing custom Babel and ESLint plugins with ASTs](https://youtu.be/VBscbcm2Mok?list=PLV5CVI1eNcJgNqzNwcs4UKrlJdhfDjshf): A 53 minute talk by [@kentcdodds](https://twitter.com/kentcdodds)
-* [babel-handbook](https://github.com/thejameskyle/babel-handbook): A guided handbook on how to use Babel and how to create plugins for Babel by [@thejameskyle](https://twitter.com/thejameskyle)
-* [Code Transformation and Linting](https://kentcdodds.com/workshops/#code-transformation-and-linting): A workshop (recording available on Frontend Masters) with exercises of making custom Babel and ESLint plugins
+- [Writing custom Babel and ESLint plugins with ASTs](https://youtu.be/VBscbcm2Mok?list=PLV5CVI1eNcJgNqzNwcs4UKrlJdhfDjshf): A 53 minute talk by [@kentcdodds](https://twitter.com/kentcdodds)


I don't know why it did this. Maybe you hadn't run the newest version of your kcd-scripts formatter on this file?

conartist6 · 2019-05-26T00:01:00Z

Never mind I figured out how to write the tests. Should be ready to go.

conartist6 · 2019-05-27T23:21:30Z

Whoops, forgot to git add a folder. Should really be ready to go.

src/index.js

conartist6 · 2019-05-29T16:30:10Z

src/index.js

+        callOptions = options[configName]
+      }
+    }
+
    try {
      // lazy-loading it here to avoid perf issues of loading it up front.


So I appreciate the comment, but maybe phobia was the right word for what influenced this decision. First, if you must synchronously load cosmiconfig no matter what I don't see how you can gain anything useful by doing it later, unless of course it turns out that babel-plugin-macros is loaded but no macros are used (which hardly seems like the case to optimize for).

Second, in optimizing for that case, you've actually hurt performance for the common case significantly. If you read the cosmiconfig docs you'll see that its caches are internal to the explorer object. You create a new explorer every time you process a file, so you are also doing a full config search every time you process a file, which is far more costly in the common case than what you were trying to avoid.

It's easy to refactor the code, but it seems like it'll be a bit tricker to fix the tests.

Actually I suppose we can have the best of both worlds if we lazy load and cache the explorer

This is done now.

conartist6 · 2019-05-29T18:33:25Z

I've added some more or less unrelated changes, but I've split them out into separate commits.

When trying to debug the tests I discovered that most tests assertion output was silently discarded.
I also found that failing objectContaining assertions where the actual object contains babel were printing output so long that they were obscuring useful output by pushing it completely out of my 1000 line terminal scrollback.

src/index.js

kentcdodds · 2019-05-30T15:41:11Z

other/docs/author.md

-
-```javascript
-// babel-plugin-macros.config.js
+// .babel.config.js


I'd prefer babel.config.js and babel-plugin-macros.config.js rather than .babel.config.js and .babel-plugin-macros.config.js

other/docs/author.md

conartist6

Are we sure we want the plugin option to override the config version. It feels more intuitive to me that it would be the other way around.

That makes sense to me too now that I think about it since the file config can be more granular -- there could be different files for different directories. I've added that change.

kentcdodds

Awesome. Thanks for this :)

kentcdodds · 2019-05-30T18:21:07Z

@all-contributors please add @conartist6 for code, tests, and docs

allcontributors · 2019-05-30T18:21:21Z

@kentcdodds

I've put up a pull request to add @conartist6! 🎉

kentcdodds · 2019-05-30T18:24:45Z

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

conartist6 · 2019-05-30T18:28:48Z

Awesome! Thanks! By the way, I realized that something I said was wrong. It definitely does make sense for macros to optimize for the case where it isn't being used, because one of its goals is to allow people who don't have control over their transpilation environment access to the power of transformations. That means it is a reasonable expectation that it will be added to quite a few projects that won't use it at all.

InvictusMB · 2019-06-04T19:53:13Z

src/index.js

@@ -88,6 +108,7 @@ function macrosPlugin(
              babel,
              interopRequire,
              resolvePath,
+              options,


@conartist6 @kentcdodds
Shouldn't the options also be passed in another applyMacros invocation?

babel-plugin-macros/src/index.js

Lines 146 to 154 in d11217e

const result = applyMacros({

path: call,

imports,

source,

state,

babel,

interopRequire,

resolvePath,

})

Could you open a new issue and/or a pull request with a failing test? Thanks!

@InvictusMB You are right. It would affect macros meant to be configured via plugin options which are imported with const { sym } = require('path/to/macro'). Nice catch.

I have created #120 with the repro and fix for this issue.

conartist6 mentioned this pull request May 25, 2019

Generate sync/async implementations from templates iter-tools/iter-tools#222

Merged

conartist6 commented May 25, 2019

View reviewed changes

conartist6 force-pushed the babel-config-options branch from 2c3520d to 0d5220a Compare May 26, 2019 00:00

Allow macro config to come from plugin options in babel config.

65ed764

conartist6 force-pushed the babel-config-options branch from 0d5220a to 65ed764 Compare May 27, 2019 22:57

kentcdodds reviewed May 28, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

conartist6 commented May 29, 2019

View reviewed changes

conartist6 force-pushed the babel-config-options branch from 1a6cae1 to c8b46a7 Compare May 29, 2019 18:29

kentcdodds reviewed May 29, 2019

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

conartist6 added 3 commits May 29, 2019 17:25

Respond to review feedback

f5f8958

Ensure failing test output is readable

071eafe

Ensure cosmiconfig conf is cached

69b15a6

conartist6 force-pushed the babel-config-options branch from c8b46a7 to 69b15a6 Compare May 30, 2019 00:30

kentcdodds reviewed May 30, 2019

View reviewed changes

Prioritize file config over plugin options config

d11217e

conartist6 commented May 30, 2019

View reviewed changes

kentcdodds approved these changes May 30, 2019

View reviewed changes

kentcdodds merged commit 796c189 into kentcdodds:master May 30, 2019

allcontributors bot mentioned this pull request May 30, 2019

docs: add conartist6 as a contributor #114

Merged

kentcdodds added the released label May 30, 2019

conartist6 deleted the babel-config-options branch May 30, 2019 18:28

InvictusMB reviewed Jun 4, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow macro config to come from plugin options in babel config. #113

Allow macro config to come from plugin options in babel config. #113

conartist6 commented May 25, 2019

conartist6 May 25, 2019

conartist6 commented May 26, 2019

conartist6 commented May 27, 2019

conartist6 May 29, 2019

conartist6 May 29, 2019

conartist6 May 30, 2019

conartist6 commented May 29, 2019

kentcdodds May 30, 2019

conartist6 left a comment

kentcdodds left a comment

kentcdodds commented May 30, 2019

allcontributors bot commented May 30, 2019

kentcdodds commented May 30, 2019

conartist6 commented May 30, 2019

InvictusMB Jun 4, 2019

kentcdodds Jun 4, 2019

conartist6 Jun 4, 2019

conartist6 Jun 5, 2019

	const result = applyMacros({
	path: call,
	imports,
	source,
	state,
	babel,
	interopRequire,
	resolvePath,
	})

Allow macro config to come from plugin options in babel config. #113

Allow macro config to come from plugin options in babel config. #113

Conversation

conartist6 commented May 25, 2019

Choose a reason for hiding this comment

conartist6 commented May 26, 2019

conartist6 commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conartist6 commented May 29, 2019

Choose a reason for hiding this comment

conartist6 left a comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented May 30, 2019

allcontributors bot commented May 30, 2019

kentcdodds commented May 30, 2019

conartist6 commented May 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment