bitttttten / jest-transformer-mdx

A jest transformer for MDX with frontMatter support
12 stars 6 forks source link

Babel config not used #1

Closed cdfa closed 4 years ago

cdfa commented 5 years ago

Hi, it seems you're very explicitly ignoring the users babel config when transforming mdx, but I do need some of my plugins so this breaks the transformation for me. May I ask why you are doing this and how you think the user should modify the used babel configuration?

bitttttten commented 4 years ago

Hey! Sorry for getting back so late, I didn't get a notification about your issue 🤔

I used the create-react-app preset as it was first intended to be used with CRA. If you're not familiar with CRA, they do not let the user customise the Babel config so there would be no config to read from. If I understand correctly, you want this plugin to read from the project's Babel config if present?

cdfa commented 4 years ago

Ah, okay. I started my project from create-react-library, but I wasn't completely satisfied with the configs.

I'd this plugin would read the project's Babel config that would be great!

bitttttten commented 4 years ago

I'll take a look on what the best approach is! There are so many ways to configure babel so I'll try and support as much as I can. It's also really difficult to detect if the project is inside an environment like CRA, or if the project is using react-scripts, etc. as these environments do not expose the babel config so I am kind of "blind".

So adding options: { babelrc: true } will not solve the issue just yet, but it might get me most of the way there.

bitttttten commented 4 years ago

So I have 2 options:

Firstly, using global options in Jest. Similar to this example: https://github.com/facebook/jest/issues/3845#issuecomment-371340479

The downside is that it'll be an awkward API to map from these into babel configs.

Secondly, letting the user configure this in the setupTestFrameworkScriptFile and exposing a setup function to run:

setupTestFrameworkScriptFile.js

import { setup } from 'jest-transformer-mdx'

setup({
        presets: [require.resolve('babel-preset-react-app')],
        babelrc: false,
        configFile: false,
})

I will just send this into the options argument of babel.transform.. so that's a downside or an upside depending on how you like your APIs.

Either could work although I lean more towards the 2nd.

cdfa commented 4 years ago

Hmm, the global option does seem the simplest way of achieving what I want, but I do also like the freedom of the second option.

Here are some thoughts: Are they mutually exclusive (e.g. can they both be implemented if the need arises?). On one hand you could start with the simple one and add the setup function later if needed, but on the other hand, if you just implement the setup you don't really need the global option anymore.

In the end it doesn't matter much to me, so choose whatever you think fits best :)

bitttttten commented 4 years ago

Alright, so check out jest-transformer-mdx@beta or jest-transformer-mdx@1.1.0-beta.

I also have this on a branch, with an updated README.

So what you have to do is set your md/mdx transformer to point to a local file. Then you import jest-transformer-mdx in this local file, set it up with the config export, and then re-export the transformer as process (that is Jest's API). See the examples and the README on that branch for more info :)

This is the tidiest solution. The latter option I mentioned in the previous comment fails since effects in setupFiles are not persistent. Each individual test file runs in isolation, so the module is also isolated.

Let me know how it goes!

Edit: hopefully I am being clear when writing the documentation. If anything is not clear just say and I will expand and adjust the docs if need be. It's tough writing good documentation 🤔

cdfa commented 4 years ago

Hmm, it doesn't seem to be working :( Maybe the babel config not being passed wasn't the problem, but I don't see what would be. The thing that isn't working is transforming import paths using babel-plugin-module-resolver. Could you have a look at this branch of my repo?

I also encountered a few other issues:

bitttttten commented 4 years ago

Ah nice catch in the docs. I'll fix that now.

Can you try adding

babelrc: false, configFile: false, filename: "null" to your options? See if this works..

config({
  babelOptions: {
    ...require("../.babelrc"),
    babelrc: false,
    configFile: false,
    filename: "null"
  }
});
cdfa commented 4 years ago

Sadly, it doesn't help. I should probably also mention that the test file is located at src/TurnReveal/docs/spec.jsx.

cdfa commented 4 years ago

Hmm, eslint is giving me warnings about these imports as well, while it didn't before. Going to look into it now.

cdfa commented 4 years ago

And they disappeared just as suddenly. Weird :confused:

bitttttten commented 4 years ago

I don't know so much about the babel and jest ecosystem, still learning it. So a small disclaimer, this might not 100% work 😅

The location of the test file shouldn't matter so much.. unless module-resolver thinks the root is now at a different location.

Try adding your plugins to your env part of the config too. Sometimes I get strange behaviour with env.X 😬 I would even play around with the root setting of the module-resolver plugin inside env.test.plugins.

module.exports = {
  presets: ["@babel/preset-env", "@babel/preset-react"],
  plugins: [
    "@babel/plugin-proposal-class-properties",
    "@babel/plugin-proposal-export-default-from",
    [
      "module-resolver",
      {
        root: ["."],
        extensions: [".js", ".jsx", ".mdx"]
      }
    ]
  ],
  env: {
    test: {
      plugins: [
        "@babel/plugin-transform-runtime",
        "@babel/plugin-proposal-class-properties",
        "@babel/plugin-proposal-export-default-from",
        [
          "module-resolver",
          {
            root: ["."],
            extensions: [".js", ".jsx", ".mdx"]
          }
        ]
      ]
    }
  }
};
cdfa commented 4 years ago

I think I figured it out: Jest transformers also receive a filename argument. If you put that in the babel options it works. (See this example). What might be even better though, is to use babel-jest transformer directly, since that's used for parsing the normal jsx files too.

cdfa commented 4 years ago

I would actually strongly suggest the second solution, since babel-jest seems to have config finding functionality built-in. At the moment, my transformer looks like this:

const { process } = require("babel-jest");

function createTransformer(src, ...args) {
  const withFrontMatter = parseFrontMatter(src);
  const jsx = mdx.sync(withFrontMatter, mdxOptions || {});
  const preBabelTransformer =
    transformer || (jsx => `import {mdx} from '@mdx-js/react';${jsx}`);

  return process(preBabelTransformer(jsx), ...args);
}

This way, the jest-transformer-mdx can also take the same transformOptions as babel-jest and it will just pass them through. If you want to go the extra mile, you could probably even extract these transformOptions from the Jest config (passed as third argument) and pass them to babel-jest yourself, so users don't have to duplicate the transformOptions.

cdfa commented 4 years ago

Also very important: have a ; after export const frontMatter = ${stringifyObject(data)}. I got syntax errors without it.

bitttttten commented 4 years ago

When I was researching how to parse the Javascript, I came across babel-jest since it made sense to use the same transformer as the majority of jest suites (and it is what CRA uses). Although the downside I found was that it doesn't load your config if you are using a tool like CRA as it doesn't expose a babel config. Since I was using CRA in all my projects where I needed this library, I went for an approach with CRA defaults. And I have since forgotten why I ended up using @babel/core in the end 🤔

If I can get it to work configless for CRA and for CRA-less projects then I am happy. I will look into extracting the transformOptions from the 3rd argument as you mentioned. That's a nice idea! I'll be investigating that this week then and will leave this PR unmerged and not release this until then :)

bitttttten commented 4 years ago

I think I am going to use babel-jest, and ship a transformer specifically for configless solutions like CRA. I.e.

    `"^.+\\.(md|mdx)$": "jest-transformer-mdx/cra"

Going to test it now and then publish.

bitttttten commented 4 years ago

Just merged https://github.com/bitttttten/jest-transformer-mdx/pull/4/

bitttttten commented 4 years ago

Can you try jest-transformer-mdx@2.0.0-beta and see if it works for you?

cdfa commented 4 years ago

Nice solution! It's almost working for me, except that syntax error I thought was caused by a missing ;, but it seems to be related to white-space? Anyway, I created #6, which should fix everything. Thank you for your help! :)

bitttttten commented 4 years ago

Strange how it was never an issue for me. I'll release it under 2.1.0. By the way, did you sign up for Hacktoberfest? Your PR will count towards it ✨

cdfa commented 4 years ago

I did. Off to a nice start :)

bitttttten commented 4 years ago

Good luck 🎉

It's released under jest-transformer-mdx@2.1.0.