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

Avoid detection if .js config file is ESM or CommonJs, just try both. #211

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

phryneas
Copy link
Member

This came up here: #187 (comment)

Because we relied on the original resolve call to determine if a file was a module or commonjs, that call would apply the normal ESM detection logic - if your package.json had type: 'module', this means that your config file could not be CommonJS anymore.

This approach now just tries to load it as ESM and if that fails tries again with CJS - we do the same for TypeScript already.

Copy link
Contributor

github-actions bot commented Sep 19, 2024

You can download the latest build of the extension for this PR here:
vscode-apollo-0.0.0-build-1726752181.pr-211.commit-24c135b.zip.

To install the extension, download the file, unzip it and install it in VS Code by selecting "Install from VSIX..." in the Extensions view.

Alternatively, run

code --install-extension vscode-apollo-0.0.0-build-1726752181.pr-211.commit-24c135b.vsix --force

from the command line.

For older builds, please see the edit history of this comment.

Comment on lines +29 to +32
url: bustFileName(specifier),
format: context.importAttributes.format,
importAttributes: context.importAttributes,
shortCircuit: true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't trust the original detection algorithm at all anymore, format is required now and we just pass this in from the outside.

@@ -0,0 +1,4 @@
{
"name": "test",
"type": "commonjs"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberately specifying the opposite here - the config file is .js with ESM contents, the package here has type: commonjs, we still want it to work.

The E2E tests will try all possible combinations on top.

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me!

@phryneas phryneas merged commit 9aa1fc1 into main Sep 19, 2024
8 checks passed
@phryneas phryneas deleted the pr/avoid-type-detection branch September 19, 2024 13:25
@github-actions github-actions bot mentioned this pull request Sep 19, 2024
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.

2 participants