Skip to content

fix importAttributes is not avaliable in old node versions#226

Merged
phryneas merged 22 commits into
apollographql:mainfrom
yesmeck:fix/import-attributes
Oct 15, 2024
Merged

fix importAttributes is not avaliable in old node versions#226
phryneas merged 22 commits into
apollographql:mainfrom
yesmeck:fix/import-attributes

Conversation

@yesmeck
Copy link
Copy Markdown
Contributor

@yesmeck yesmeck commented Oct 11, 2024

fix #225

Tested with VSCode 1.91.1 (Node.js 20.9.0) and VSCode 1.94.0 (Node.js 20.16.0).

@apollo-cla
Copy link
Copy Markdown

@yesmeck: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@yesmeck yesmeck force-pushed the fix/import-attributes branch from 913e6ae to 70d5deb Compare October 11, 2024 10:09
@yesmeck yesmeck marked this pull request as draft October 11, 2024 13:57
@phryneas
Copy link
Copy Markdown
Member

I'm merging in main, as I've added a CI run for 1.90.0 there.

Comment thread src/language-server/config/cache-busting-resolver.js Outdated
@yesmeck yesmeck marked this pull request as ready for review October 15, 2024 10:42
Comment thread src/language-server/config/cache-busting-resolver.js
Comment thread src/language-server/config/cache-busting-resolver.js Outdated
Comment thread src/language-server/config/cache-busting-resolver.js Outdated
@phryneas
Copy link
Copy Markdown
Member

Generally, could you please add some comment to document that once we hit a minimum of 1.92, most of these workarounds can be removed again, and reference the issue you started?
So we don't confuse future maintainers :)

Comment thread src/language-server/config/cache-busting-resolver.js
Comment thread src/language-server/config/cache-busting-resolver.js
Comment thread src/language-server/config/cache-busting-resolver.js
@phryneas
Copy link
Copy Markdown
Member

Okay, once the tests come back happy all that's left is a changeset - could you please run npx changeset and add a changeset for a patch-level change? :)

@phryneas
Copy link
Copy Markdown
Member

Huh, that test runs fine for me locally. I'll investigate.

@yesmeck
Copy link
Copy Markdown
Contributor Author

yesmeck commented Oct 15, 2024

It seems we can't set contents on importAssertions. It's so weird that it was working for me.

@phryneas
Copy link
Copy Markdown
Member

This is just super weird :(
Any more ideas?
Putting contents into as too? 😢

@yesmeck
Copy link
Copy Markdown
Contributor Author

yesmeck commented Oct 15, 2024

Can you rerun CI? It works for me locally now. We need to set both ⁠format and ⁠contents on the assert object, even though we can’t access the format from ⁠importAssertions, and the order of ⁠format and ⁠contents as keys also matters.

Copy link
Copy Markdown
Member

@phryneas phryneas left a comment

Choose a reason for hiding this comment

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

And we're green.
Thank you very much for your hard work with this!

@phryneas phryneas merged commit 57c51c8 into apollographql:main Oct 15, 2024
@github-actions github-actions Bot mentioned this pull request Oct 15, 2024
@yesmeck yesmeck deleted the fix/import-attributes branch October 15, 2024 13:26
@phryneas
Copy link
Copy Markdown
Member

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.

importAttributes not available in old node versions

3 participants