[1.0] Fix graphql compiler on typescript #949

fabien0102 · 2017-05-10T16:54:11Z

Bug description

Actually graphql queries are compiled with a complexe ast parser and a babel plugin. Due to typescript files don't use any babel loader and not finished by .js they are totally ignored in this process.

What I did

Just add a babel into typescript loader (and hijack query from js loader), and deal with .ts|.tsx|.jsx files for queries parsing

Integration test on my starter

It's usefull to parse any graphql queries

gatsbybot · 2017-05-10T16:56:42Z

Deploy preview failed.

Built with commit adc55d074820650df7ec1dbd46976860beeb5c4b

https://app.netlify.com/sites/using-drupal/deploys/591345b4cf321c336be384d3

KyleAMathews · 2017-05-10T16:57:15Z

Deploy preview failed.

Built with commit adc55d074820650df7ec1dbd46976860beeb5c4b

https://app.netlify.com/sites/gatsbyjs/deploys/591345b4cf321c336be384cd

KyleAMathews · 2017-05-10T16:58:32Z

Deploy preview failed.

Built with commit adc55d074820650df7ec1dbd46976860beeb5c4b

https://app.netlify.com/sites/gatsbygram/deploys/591345b4cf321c336be384d0

gatsbybot · 2017-05-10T16:59:03Z

Deploy preview ready!

Built with commit 7cc009e

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

KyleAMathews · 2017-05-10T17:01:03Z

Deploy preview ready!

Built with commit 7cc009e

https://deploy-preview-949--gatsbyjs.netlify.com

KyleAMathews · 2017-05-10T17:02:30Z

Deploy preview ready!

Built with commit 7cc009e

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

jquense · 2017-05-10T17:07:24Z

packages/gatsby-plugin-typescript/src/gatsby-node.js

@@ -19,7 +19,10 @@ module.exports.modifyWebpackConfig = ({ config }, { compilerOptions }) => {
  const opts = { compilerOptions: copts, transpileOnly: true }
  config.loader(`typescript`, {
    test,
-    loader: `ts-loader?` + JSON.stringify(opts),
+    loaders: [
+      `babel?${JSON.stringify(config._loaders.js.config.query)}`,


This may do more than you want here. You really only want to run the babel query extractor plugin. This will run the file through all the babel transforms (admittedly most won't matter after tsc runs).

Honestly you probably better to just leave this off entirely, the plugin isn't important to running anything, it's really more of a minification step, as the queries aren't used a runtime it's better to just remove the code.

another option is to move the babel plugin to a proper package that can be consumed by plugins directly

I know it's a bit overkill, I just want to avoid any futur missed babel transformer (we never think to typescript module ^^).

I'm not a big fan with another package for babel, we have already manyyyyy packages (and it introduce some tricky part for anyone want use gatsby without starter)

Just for information:

// During a `yarn build` on my starter config._loaders.js.config.query = { "plugins": [ "D:\\Projets\\gatsby-starter\\node_modules\\gatsby\\dist\\utils\\babel-plugin-extract-graphql.js", "D:\\Projets\\gatsby-starter\\node_modules\\babel-plugin-add-module-exports\\lib\\index.js", "D:\\Projets\\gatsby-starter\\node_modules\\babel-plugin-add-module-exports\\lib\\index.js", "D:\\Projets\\gatsby-starter\\node_modules\\babel-plugin-transform-object-assign\\lib\\index.js" ], "presets": [ "D:\\Projets\\gatsby-starter\\node_modules\\babel-preset-es2015\\lib\\index.js", "D:\\Projets\\gatsby-starter\\node_modules\\babel-preset-stage-0\\lib\\index.js", "D:\\Projets\\gatsby-starter\\node_modules\\babel-preset-react\\lib\\index.js" ], "cacheDirectory": true }

I will try to only take graphql-extract plugin ;) (but tomorow, it's late ^^)

jquense · 2017-05-10T17:08:54Z

packages/gatsby/src/query-runner/query-compiler.js

@@ -50,7 +50,8 @@ class Runner {
  }

  async parseEverything() {
-    let files = await globp(`${this.baseDir}/**/*.js`)
+    let files = await globp(`${this.baseDir}/**/*.+(t|j)s?(x)`)
+    files = files.filter(d => !d.match(/(\.stories\.|\.test\.|\.d\.)/))


this (I think) is too prescriptive. you'll undoubtably catch actual source files. perhaps it makes sense to make a a config option for "ignore paths" or something that is specifiable in the gatsby-config

It's based on classic patterns of jest/storybook/typescript declaration files 😉 ignorePath can be a cool options but I don't want to provide useless option as it's commun patterns.

@KyleAMathews What do you thing on this issue?

I understand why you chose it :) my point is only that conventions only meet some peoples needs. for instance in my own projects this wouldn't catch any of my test files or stories. More importantly tho it will likely silently filter real source files which is a different version of the problem of this problem

This line is only to avoid a warning on .d.ts file in my current project, I've just add stories and tests to optimize a little bit more ;) (so it's not really important for now if any test file are not filtered)

I agree with @jquense that by default we shouldn't ignore anything.

Shouldn't this just be globbing the src directory though?

All this files are in src (one folder for one module, with tests beside component)
My only issue is with d.ts (needed for js module and global var as graphql) I can remove test and stories if you want ;) It was just to avoid useless processing

fabien0102 · 2017-05-10T17:25:20Z

BTW I don't try to have a project with coffeescript, but I think it will have the same issues

KyleAMathews · 2017-06-01T19:03:04Z

@fabien0102 what's the status on Gatsby + Typescript?

fabien0102 · 2017-06-01T20:13:36Z

Sorry, I was very busy these last weeks, I will update this PR during next week ;)

gatsbybot · 2017-06-01T20:27:31Z

No worries! Get to it when you can!

…

On Thu, Jun 1, 2017, 1:13 PM Fabien BERNARD ***@***.***> wrote: Sorry, I was very busy these last weeks, I will update this PR during next week ;) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#949 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYdmDeJpnHaLgDtahPx9UHcDp-tbeXiXks5r_xt3gaJpZM4NW8dY> .

…iler

This is to avoid conflit with `@types/{packageName}` pattern into packages.json

jquense · 2017-06-09T16:36:32Z

packages/gatsby/src/schema/create-key.js

@@ -1,3 +1,3 @@
 const regex = new RegExp(`[^a-zA-Z0-9_]`, `g`)

-module.exports = key => key.replace(regex, `___`)
+module.exports = key => key.replace(`@`, ``).replace(regex, `___`)


I don't think removing this is the best option, you can get namespace conflcits on fields without the @ that aren't resolvable.

I did fix this in two ways tho in #1136

I will cherry-pick your commit 😄 For instant I just try to have a functionnal gatsby env, I will clean this PR before remove [WIP] tag 😉

@jquense I've cherry-pick your two commits in my branch and…

It's not working for me…

looks like you didn't change the queries to adjust to the commits? or maybe i didn't update the examples. it replaces characters with a single _ instead of 3 so frontmatter_updateRole etc

Indeed! My bad

…iler

fabien0102 · 2017-06-14T21:18:20Z

After many pains, this PR is fully working on my starter integration (fabien0102/gatsby-starter#24)

KyleAMathews

Looking good! Sorry for all the API changes but glad you were able to get it working again.

I'm not in love with adding stuff to globs, etc. to support additional languages but this is the cheapest way to get this working right now for sure. This is what we did in 0.x and it worked out alright. When we add support for more compile-to-js languages we can debate a better API then and update this plugin accordingly.

KyleAMathews · 2017-06-15T07:02:11Z

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

@@ -15,14 +14,6 @@ import type { DocumentNode, DefinitionNode } from "graphql"

 const readFileAsync = Bluebird.promisify(fs.readFile)

-function getAssignedIdenifier(path) {


was this not being used?

My VSCode tell me that is not used, but you can recheck ;) (I try to remove unused code when I see it)

@jquense you know what this was used for?

well if no code is calling it whatevers. We can recover it later if it's needed for something.

KyleAMathews · 2017-06-15T16:15:28Z

Thanks again @fabien0102 for holding the line on Typescript :-D

fabien0102 · 2017-06-15T17:20:55Z

@KyleAMathews When do you plan a new release? I need a "real" version for my starter 😉

KyleAMathews · 2017-06-15T17:24:34Z

As soon as I finish #1174 going to do a beta release!

fabien0102 · 2017-06-15T18:40:07Z

Perfect!

fabien0102 added 6 commits May 10, 2017 17:17

Remove unused var

6172024

Fix indent

764b733

Add babel to typescript plugin

5401cda

It's usefull to parse any graphql queries

Let parseEverything really parse everything

9580915

Exclude tests, declarations and stories

347abf4

Run yarn format

9f60674

fabien0102 force-pushed the 1.0-fix-graphql-compiler branch from adc55d0 to 9f60674 Compare May 10, 2017 16:55

jquense reviewed May 10, 2017

View reviewed changes

Don't filter tests and stories

5cd6f14

fabien0102 added 4 commits June 8, 2017 19:34

Merge remote-tracking branch 'upstream/1.0' into 1.0-fix-graphql-comp…

6951297

…iler

Fix building schema console.time

893a9d9

Fix create-path on windows

4bca4a0

Deal with @ into keys

353b44b

This is to avoid conflit with `@types/{packageName}` pattern into packages.json

fabien0102 changed the title ~~[1.0] Fix graphql compiler on typescript~~ [1.0] [WIP] Fix graphql compiler on typescript Jun 9, 2017

jquense reviewed Jun 9, 2017

View reviewed changes

fabien0102 added 3 commits June 11, 2017 11:21

Add tests and refactor create-path

32e7508

Fix types of gatsby-link

698acc8

Add ts and tsx into query-watcher

7f6229a

fabien0102 mentioned this pull request Jun 12, 2017

Moar bugs #1136

Merged

fabien0102 added 2 commits June 14, 2017 22:23

Merge remote-tracking branch 'upstream/1.0' into 1.0-fix-graphql-comp…

c8e05c1

…iler

Only take babel-plugin-extract-graphql plugin

7cc009e

fabien0102 changed the title ~~[1.0] [WIP] Fix graphql compiler on typescript~~ [1.0] Fix graphql compiler on typescript Jun 14, 2017

fabien0102 mentioned this pull request Jun 14, 2017

Update gatsby fabien0102/gatsby-starter#24

Merged

KyleAMathews reviewed Jun 15, 2017

View reviewed changes

KyleAMathews merged commit 1639377 into gatsbyjs:1.0 Jun 15, 2017

fabien0102 deleted the 1.0-fix-graphql-compiler branch October 4, 2017 08:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1.0] Fix graphql compiler on typescript #949

[1.0] Fix graphql compiler on typescript #949

fabien0102 commented May 10, 2017 •

edited

Loading

gatsbybot commented May 10, 2017

KyleAMathews commented May 10, 2017

KyleAMathews commented May 10, 2017

gatsbybot commented May 10, 2017 •

edited

Loading

KyleAMathews commented May 10, 2017 •

edited by gatsbybot

Loading

KyleAMathews commented May 10, 2017 •

edited by gatsbybot

Loading

jquense May 10, 2017

fabien0102 May 10, 2017

fabien0102 May 10, 2017

jquense May 10, 2017

fabien0102 May 10, 2017

jquense May 10, 2017

fabien0102 May 10, 2017

KyleAMathews May 10, 2017

fabien0102 May 10, 2017

fabien0102 commented May 10, 2017

KyleAMathews commented Jun 1, 2017

fabien0102 commented Jun 1, 2017

gatsbybot commented Jun 1, 2017 via email

jquense Jun 9, 2017

fabien0102 Jun 11, 2017

fabien0102 Jun 11, 2017

jquense Jun 12, 2017

fabien0102 Jun 12, 2017

fabien0102 commented Jun 14, 2017

KyleAMathews left a comment

KyleAMathews Jun 15, 2017

fabien0102 Jun 15, 2017

KyleAMathews Jun 15, 2017

KyleAMathews Jun 15, 2017

KyleAMathews commented Jun 15, 2017

fabien0102 commented Jun 15, 2017

KyleAMathews commented Jun 15, 2017

fabien0102 commented Jun 15, 2017

		@@ -15,14 +14,6 @@ import type { DocumentNode, DefinitionNode } from "graphql"

		const readFileAsync = Bluebird.promisify(fs.readFile)

		function getAssignedIdenifier(path) {

[1.0] Fix graphql compiler on typescript #949

[1.0] Fix graphql compiler on typescript #949

Conversation

fabien0102 commented May 10, 2017 • edited Loading

Bug description

What I did

gatsbybot commented May 10, 2017

KyleAMathews commented May 10, 2017

KyleAMathews commented May 10, 2017

gatsbybot commented May 10, 2017 • edited Loading

KyleAMathews commented May 10, 2017 • edited by gatsbybot Loading

KyleAMathews commented May 10, 2017 • edited by gatsbybot Loading

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

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

Choose a reason for hiding this comment

fabien0102 commented May 10, 2017

KyleAMathews commented Jun 1, 2017

fabien0102 commented Jun 1, 2017

gatsbybot commented Jun 1, 2017 via email

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

Choose a reason for hiding this comment

fabien0102 commented Jun 14, 2017

KyleAMathews left a comment

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

Choose a reason for hiding this comment

KyleAMathews commented Jun 15, 2017

fabien0102 commented Jun 15, 2017

KyleAMathews commented Jun 15, 2017

fabien0102 commented Jun 15, 2017

fabien0102 commented May 10, 2017 •

edited

Loading

gatsbybot commented May 10, 2017 •

edited

Loading

KyleAMathews commented May 10, 2017 •

edited by gatsbybot

Loading

KyleAMathews commented May 10, 2017 •

edited by gatsbybot

Loading