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: update hook signature #10410

Merged
merged 3 commits into from
Dec 17, 2024
Merged

fix: update hook signature #10410

merged 3 commits into from
Dec 17, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Dec 17, 2024

Description (required)

The documentation is incorrect, and the updateConfig is for vite, not Astro.

I wonder if we should change its name...

Related issues & labels (optional)

I noticed the mistake in withastro/astro#12372

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit 4aed803
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/6761836522032e0008d14645
😎 Deploy Preview https://deploy-preview-10410--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Contributor

astrobot-houston commented Dec 17, 2024

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
en/reference/integrations-reference.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing @ematipico !

@sarah11918 sarah11918 added code snippet update Updates a code sample: typo, outdated code etc. Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! labels Dec 17, 2024
@rbsummers
Copy link

rbsummers commented Dec 17, 2024

Hi there! I think this PR, while necessary, won't close the referred issue because we need some changes in the astro package to have the config matter for vite.build. I've opened a PR to do that but for now I have only manually tested it, I'll need more time to figure out the testing setup

See also https://github.com/withastro/astro/pull/12763/files

@sarah11918 sarah11918 added merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) and removed Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! labels Dec 17, 2024
@sarah11918
Copy link
Member

Thank you @rbsummers -- can you let me know:

Is the proposal in this PR a change to correct, existing behavior or does this change rely on the linked PR being released to be correct?

Docs PRs are merged independently of feature releases, and publish live immediately. So, it's important to know whether this is a fix to incorrect (current, published) documentation, or whether this change is only correct when the linked PR is released in the next patch version of Astro.

It's also OK to have multiple docs PRs if, for example, something is immediately wrong and needs correcting now, but then another PR will be needed to accompany the linked PRs release. I'm just not sure the state of this feature right now, and it would help me to know so I can make sure we're merging at the correct time! 🙌

@rbsummers
Copy link

rbsummers commented Dec 17, 2024

Hi @sarah11918,

It was a tricky situation because the feature was implemented (the function updateConfig was in the code, probably because it mimicked some other function available & well documented in a different hook), but it did nothing and it was not properly documented for this other hook. My PR (which @ematipico merged a bit ago, thank you!) fixed the first point, and this one addresses the second.

If merging this makes the doc change immediately, maybe we need to hold on a bit until the version with the fix (5.0.10 I guess?) is released and then merge.

@ematipico
Copy link
Member Author

If merging this makes the doc change immediately, maybe we need to hold on a bit until the version with the fix (5.0.10 I guess?) is released and then merge.

Fixing the API signature should be done regardless of the fix of the code.

@sarah11918
Copy link
Member

@ematipico You have my approval to merge whatever is correct now! I just wasn't sure what was happening. 😄

I agree, if this PR is entirely correct now, my suggestion would be to merge it!

@ematipico
Copy link
Member Author

I updated the PR description to mention that I noticed the mistake in an issue, but PR won't fix it the issue in core.

@ematipico ematipico merged commit 8c233d2 into main Dec 17, 2024
10 checks passed
@ematipico ematipico deleted the chore/update-hook branch December 17, 2024 15:15
ArmandPhilippot added a commit to ArmandPhilippot/astro-docs that referenced this pull request Dec 17, 2024
yanthomasdev added a commit that referenced this pull request Dec 17, 2024
See #10410

Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
Nin3lee added a commit to Nin3lee/docs that referenced this pull request Jan 14, 2025
liruifengv added a commit that referenced this pull request Jan 16, 2025
* i18n(zh-cn): Update `mixed-content-data-collection-error.mdx`

* Update `mixed-content-data-collection-error.mdx`

* Update `content-collection-type-mismatch-error.mdx`

* [WIP]i18n(zh-cn): Add `v5.mdx`

* [WIP] Update `v5.mdx`

* Update v5.mdx

* fix: broken link in `content-collections.mdx`

* fix: broken link in `content-schema-contains-slug-error.mdx`

* fix: part of broken link in `v5.mdx`

* Update `integrations-reference.mdx`

* Update `integrations-reference.mdx`

* fix: broken link in `v5.mdx`

* fix: broken link in `integrations-reference.mdx`

* fix: broken link in `adapter-reference.mdx`

* Update `integrations-reference.mdx` from #10410

* Update src/content/docs/zh-cn/reference/errors/content-collection-type-mismatch-error.mdx

* Update src/content/docs/zh-cn/reference/errors/mixed-content-data-collection-error.mdx

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

* Update src/content/docs/zh-cn/guides/content-collections.mdx

* Update src/content/docs/zh-cn/reference/errors/content-collection-type-mismatch-error.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/reference/errors/mixed-content-data-collection-error.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/reference/errors/content-schema-contains-slug-error.mdx

* Update src/content/docs/zh-cn/reference/errors/mixed-content-data-collection-error.mdx

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update src/content/docs/zh-cn/guides/upgrade-to/v5.mdx

Co-authored-by: liruifengv <liruifeng1024@gmail.com>

* Update `v4.mdx` from discussion

#10702 (comment)

---------

Co-authored-by: liruifengv <liruifeng1024@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code snippet update Updates a code sample: typo, outdated code etc. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants