Allow plugins to return custom @babel/parser options in preprocessSource

[dennari]

No description provided.

[gatsbybot]

Deploy preview for using-drupal ready!

Built with commit f5186b8

https://deploy-preview-6209--using-drupal.netlify.com

[gatsbybot]

Deploy preview for gatsbygram ready!

Built with commit f5186b8

https://deploy-preview-6209--gatsbygram.netlify.com

[pieh]

I feel like there should be seperate API for passing custom BABYLON_OPTIONS options. I didn't understand this change in isolation and needed to look on your other PR to get what this supposed.

[jquense]

yeah I think ideally a plugin would use the babel options api for this or provide custom logic in the preprocessSource api

[dennari]

@pieh, @jquense I agree that ideally there would be a separate API. However I don't see the harm in going with this for the time being. Did you see the discussion here: #6174? @m-allanson, what are your thoughts?

Also, looking way forward, perhaps the solution I outline in #6206 is the way to go in the long term.

[jquense]

I think you currently can do this already via the babel api no? I f not i think it'd be more productive to add that API than extend this one :)

[dennari]

The babel api is for configuring babel plugins, not the babel parser plugins. These are hard coded.

In my estimation the benefits of implementing a new api for this is just not worth the trouble. Really the only reason I can see for wanting to change the default parser plugins is the case of typescript (the flow and the typescript plugins can't be enabled simultaneously).

I'd say the choice is between the solution in this PR or the one in #6174, which is more complicated and less performant. Also note that the typescript plugin is broken until one of these gets merged.

[resir014]

Any updates on this?

[pieh]

In my estimation the benefits of implementing a new api for this is just not worth the trouble.

Doing that isn't really difficult - You literally just add `apiRunner('') and add docs to , so only reason not to do that would be implications that this API should be stable - but this also true with modyfing already existing API.

How about doing something like this:
add addBabylonConfig (bad name, let's show example to explain what I mean) that would be run just once and would be used by plugin like this:

export function addBabylonConfig() {
  return [
    {
      test: /\.tsx?$/,
      options: PARSER_OPTIONS
    }
  ]
}

and then let gatsby pick proper parser options depending on extension and doing fallback to default if none of tests passed by plugins matched? PARSER_OPTIONS would be like in related PR from @dennari ( https://github.com/gatsbyjs/gatsby/pull/6208/files#diff-bea7c172b1a0ade077b2d12ae5603b4dR18 )

[jquense]

I'm not sure i understand when these options would be used @pieh. From what i tell internal babylon.parse operations (for graphql extranction etc.) would use the preprocessSource api. Is the problem with that api the slow-ness of transforming to JS then parsing? If so maybe giving the option for preprocessSource to return an AST is enough. Ideally we probably want limit a plugins coupling to Gatsby's babylon usage since it really limits plugin upgrades in lockstep with a gatsby version including the newest parser right (A problem the relay compiler has i might add)?

[jeffwillette]

IMO, the question really boils down to if there is any broader use case for having to switch the PARSER_OPTIONS. If the only foreseeable use case is for typescript then maybe gatsby should just be swapping it out for TS files without the need for a public API?

[pieh]

This PR is exactly about transforming TS to JS and then parsing that to ast , that it has unneeded steps that could be skipped - we just argued what's the best way :)

My idea was to pick appropiate parserOptions to be used here

gatsby/packages/gatsby/src/internal-plugins/query-runner/file-parser.js

Line 92 in 7ed8da9

ast = babylon.parse(fileStr, BABYLON_OPTIONS)

But I definitely get your point. Returning AST seems good - this does make sense in context of preprocessSource API name, which was source my biggest gripe with current solution in this PR (that it didn't really make sense that we would return parser options from preprocessSource, which caused unnecessary cognitive load when reading it).

In any case, I won't oppose any solution here - my initial complaint now seems not productive and only blocked this from moving forward.

[jquense]

If the only foreseeable use case is for typescript then maybe gatsby should just be swapping it out fir TS files without the need for a public API?

I actually think maybe this is the best short term idea. I recently did this on another project (here) and it works nicely. My only concern here is that adding a sub optimal api for this is likely to lead us down a frustrating road as things get more complicated for other usecases, and so it'd be great to avoid it if possible.

@pieh what do you think about just supporting Typescript "natively", since we effectively do that for flow now, there are only two real options in this space, and it's TS is the most popular anyway

[pieh]

IMO, the question really boils down to if there is any broader use case for having to switch the PARSER_OPTIONS. If the only foreseeable use case is for typescript then maybe gatsby should just be swapping it out fir TS files without the need for a public API?

Very good point and I think it's best solution for now - we can always revisit it in the future without being tied to new or change APIs this way.

@dennari What do You think?

[dennari]

Both solutions, hardcoding another set of options for .tsx? files or returning an AST, seem like good solutions to me. The only one I'm hesitant with is adding another public api for this, since as mentioned, there's probably not much use for it. Also, IMHO the longer term solution to this and related problems is being able to reuse the same babel configuration here that gets used when loading with webpack (#6206).

[dennari]

Since hardcoding the typescript support is the easier solution here, and if we're willing to do that, I'd probably choose that. I can create a new PR for that if we so agree.

[pieh]

There is another place we have yet another config for parser options -

gatsby/packages/gatsby/src/bootstrap/resolve-module-exports.js

Lines 27 to 43 in 2acc8b9

	const babylonOpts = {
	sourceType: `module`,
	allowImportExportEverywhere: true,
	plugins: [
	`jsx`,
	`doExpressions`,
	`objectRestSpread`,
	`decorators`,
	`classProperties`,
	`exportExtensions`,
	`asyncGenerators`,
	`functionBind`,
	`functionSent`,
	`dynamicImport`,
	`flow`,
	],
	}

Might be worth to consolidate those?

[jquense]

yeah we need a "parse util" gatsby can export some plugins do this also, e.g. react-docgen

[KyleAMathews]

👍 to hardcoding things. APIs are hard to do well and can be expensive to maintain (there's docs, support, bugs, etc). so we should avoid them when possible.

[dennari]

Alright, created a new PR for this: #6335