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

fix(gatsby-parcel-config): Adjust dependencies #36583

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

ebuildy
Copy link
Contributor

@ebuildy ebuildy commented Sep 9, 2022

Description

Add YAML transformer parcel

Related Issues

I had to add YAML transformer parcel to fix build error:

 ERROR #11901  COMPILATION

Failed to compile Gatsby files (@parcel/core):

Could not determine version of @parcel/transformer-yaml in node\_modules/gatsby-parcel-config/lib/index.json. Either include it in "dependencies" or "parcelDependencies"..

File path: XXXX/node_modules/gatsby-parcel-config/package.json

not finished compile gatsby files - 0.392s

Dont know why, I had to add YAML transformer parcel to fix build error:

```
 ERROR gatsbyjs#11901  COMPILATION

Failed to compile Gatsby files (@parcel/core):

Could not determine version of @parcel/transformer-yaml in node\_modules/gatsby-parcel-config/lib/index.json. Either include it in "dependencies" or "parcelDependencies"..

File path: XXXX/node_modules/gatsby-parcel-config/package.json

not finished compile gatsby files - 0.392s
```
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 9, 2022
@marvinjude marvinjude added topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 9, 2022
@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label labels Sep 12, 2022
@LekoArts
Copy link
Contributor

Please allow edits by maintainers, as more changes are necessary in this PR (we need to change more dependencies)

image

@LekoArts LekoArts added the status: awaiting author response Additional information has been requested from the author label Sep 12, 2022
@ebuildy
Copy link
Contributor Author

ebuildy commented Sep 14, 2022

it was set already

@LekoArts LekoArts removed the status: awaiting author response Additional information has been requested from the author label Sep 15, 2022
@LekoArts LekoArts changed the title fix(deps): add yaml transformer parcel fix(gatsby-parcel-config): Adjust dependencies Sep 19, 2022
@LekoArts
Copy link
Contributor

We shouldn't handle yaml or any other files really, so instead of doing your change of adding the dep, we should remove the yaml transformer (and others) from the config.

tyhopp
tyhopp previously approved these changes Sep 19, 2022
@LekoArts LekoArts merged commit 9e3f160 into gatsbyjs:master Sep 19, 2022
@ebuildy
Copy link
Contributor Author

ebuildy commented Sep 19, 2022

@LekoArt agree, I found weird to have to do this fix, but how to do that on a gatsby application?

And just curious, how could I use this fix in my package.json please ?

Many thanks

@miensol
Copy link

miensol commented Sep 21, 2022

@LekoArts would you mind sharing what's the reasoning behind your comment #36583 (comment) ?

@LekoArts
Copy link
Contributor

@ebuildy You can use gatsby@next or wait for the stable release on 2022/09/27 (we release every two weeks).

@miensol We use Parcel to transform TypeScript to JavaScript. It shouldn't do more and the TypeScript should be "valid" code for Node.js as in: For example Node by default doesn't support just loading YAML files, one has to use https://www.npmjs.com/package/yaml. So if you want, you can compare behavior to the typescript compiler.

@miensol
Copy link

miensol commented Sep 21, 2022

Thanks @LekoArts .
The reason I asked was that I had tried what is described in the documentation in a slightly different context. That is in the context of gatsby-config as opposed to page context. I understand now where my error was. I wonder though if it will not confuse more users.

@LekoArts
Copy link
Contributor

Historically speaking the gatsby-config.js always worked like that, only with the introduction of Parcel to compile the TS files this changed (until now). With a JS config Node would have yelled at you I'd assume.

@omardeangelis
Copy link

omardeangelis commented Sep 28, 2022

How can we handle now the import of json-files used for handle redirect ? In the doc this is the best practice described to handle this ? Cause i get No transformers found for __redirects.json__ when run my dev server build

@tyhopp
Copy link
Contributor

tyhopp commented Sep 29, 2022

@omardeangelis can you open an issue with a minimal reproduction? I just tried importing a JSON file in gatsby-config with gatsby@next installed (which includes this change) and I don't hit an error.

@openscript
Copy link
Contributor

I'm having similar problems:

root ➜ /workspaces/gatsby-starter-dogmatism (master ✗) $ yarn start
yarn run v1.22.18
$ yarn run develop
$ gatsby develop

 ERROR #11901  COMPILATION

Failed to compile Gatsby files (@parcel/core):

No transformers found for __content/i18n/zh-CN.json__..

File path: /workspaces/gatsby-starter-dogmatism/node_modules/gatsby-parcel-config/lib/index.json

not finished compile gatsby files - 1.083s

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
root ➜ /workspaces/gatsby-starter-dogmatism (master ✗) $ yarn why gatsby
yarn why v1.22.18
[1/4] Why do we have the module "gatsby"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "gatsby@4.25.0-next.1"
info Has been hoisted to "gatsby"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "33.61MB"
info Disk size with unique dependencies: "33.61MB"
info Disk size with transitive dependencies: "33.61MB"
info Number of shared dependencies: 392
Done in 1.24s.

How to resolve this?

@omardeangelis
Copy link

@omardeangelis can you open an issue with a minimal reproduction? I just tried importing a JSON file in gatsby-config with gatsby@next installed (which includes this change) and I don't hit an error.

Try to import inside gatsby.node.ts
I'm on "gatsby": "^4.24.0"

@that1matt
Copy link
Contributor

that1matt commented Oct 4, 2022

I built a simple workaround for this, as we have a monorepo where we import specific keys from a .json file.

Added this command as a postinstall script at our global package.json.
node --experimental-specifier-resolution=node ./scripts/convert-json.mjs

Here is the contents of the convert-json.mjs

import fs from 'fs';

import prettier from 'prettier';

import data from './data.json';

const repos = ['mono', 'repo', 'names'];

const convertJSON = () => {
  const jsData = `module.exports = ${JSON.stringify(data)}`;
  const formattedData = prettier.format(jsData, { singleQuote: true, filepath: '../.prettierrc.js' });

  repos.map(repo => {
    fs.writeFileSync(`./${repo}/converted-data.js`, formattedData);
  });
};

convertJSON();

I added the extra prettier step so we do not get any conflicts with our eslint/prettier config.

This isn't ideal, but something quick while we wait for a more permanent solution from gatsby.

@LekoArts
Copy link
Contributor

LekoArts commented Oct 5, 2022

Node by default doesn't support importing JSON files in ESM (see https://www.stefanjudis.com/snippets/how-to-import-json-files-in-es-modules-node-js/)

You can use fs for example:

import fs from "fs"
const data = JSON.parse(fs.readFileSync("./data.json", "utf8"))

Bildschirmfoto 2022-10-05 um 09 28 48

However, I understand that it's an easy to have "Quality of Life" improvement so I'll re-add the JSON transformer.

@LekoArts
Copy link
Contributor

LekoArts commented Oct 5, 2022

I've put up #36748

@omardeangelis
Copy link

Node by default doesn't support importing JSON files (see https://www.stefanjudis.com/snippets/how-to-import-json-files-in-es-modules-node-js/)

Thanks, think that the problem is that this PR and the official docs say different things.

Can i ask you what are the benefits of the JSON transformer removal ? pure curiosity

@LekoArts
Copy link
Contributor

LekoArts commented Oct 5, 2022

Thanks, think that the problem is that this PR and the official docs say different things.

This is using CommonJS with require which works with JSON files. So the instructions are correct. import in ESM doesn't work. Anyhow, as you can see in the above PR we'll re-add it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants