Skip to content

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

Merged
phryneas merged 4 commits into
mainfrom
pr/avoid-type-detection
Sep 19, 2024
Merged

Avoid detection if .js config file is ESM or CommonJs, just try both.#211
phryneas merged 4 commits into
mainfrom
pr/avoid-type-detection

Conversation

@phryneas
Copy link
Copy Markdown
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.

@github-actions
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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