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: use require instead if the ESM import statement #11331

Closed
wants to merge 1 commit into from
Closed

fix: use require instead if the ESM import statement #11331

wants to merge 1 commit into from

Conversation

DanielRuf
Copy link
Contributor

Description

This fixes an issue with the build script.

In the dev mode it seems we have the babel transform plugin for the engine and dynamic require / import.

But the build does not and we have to use require. In fact the code in this file uses exports but not the require. I guess this was overseen.

We should use the CJS way to use them (require) instead of the ESM one (import).

https://github.com/gatsbyjs/gatsby/blame/master/packages/gatsby/src/internal-plugins/query-runner/pages-writer.js#L97

https://github.com/gatsbyjs/gatsby/blame/master/packages/gatsby/src/internal-plugins/query-runner/pages-writer.js#L104

Related Issues

#11198

@DanielRuf DanielRuf requested a review from a team as a code owner January 27, 2019 12:49
@DanielRuf
Copy link
Contributor Author

Hm, does my change break the tests? It seems so.

@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

It seems like You are changing dynamic import into static one here. What about doing this other way and making this entirely ESM:
exports.data => export const data?
It's also suprising that it break things for some people, but not for others :/

@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

Do you have example project that is failing currently that I could investigate?

@DanielRuf
Copy link
Contributor Author

@pieh see #11198 (comment) for a repository for testing the problem. All previous gatsby releases have this issue so it might be because of another dependency.

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Jan 28, 2019

It seems a new install (using Yarn) does not have this issue. I'll do a diff between the working and not working setup.

@DanielRuf
Copy link
Contributor Author

DanielRuf commented Jan 28, 2019

exports.data => export const data?

This does not change anything, the needed runtime transforms are still not applied.

I think it is different between Yarn and npm.

@pieh
Copy link
Contributor

pieh commented Jan 28, 2019

Yeah, I cloned the reproduction and had same problem when installing with npm - but after wiping lock file and reinstalling node_modules with npm problem went away, so to me it seems like some dependency issues

@DanielRuf
Copy link
Contributor Author

Seems so. But so far we do not know the exact cause. I'm closing the PR.

@DanielRuf DanielRuf closed this Jan 28, 2019
@DanielRuf DanielRuf reopened this Jan 29, 2019
@DanielRuf
Copy link
Contributor Author

It seems it still happens with npm, but not Yarn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants