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

add package.json hint #6186

Merged
merged 6 commits into from
Jan 18, 2024
Merged

add package.json hint #6186

merged 6 commits into from
Jan 18, 2024

Conversation

nadar
Copy link
Contributor

@nadar nadar commented Jan 7, 2024

For non-super-duper experts, it can be helpful to have a hint that those files must be exported, see my struggle and the helpful answer of @Fryuni

withastro/astro#7721 (comment)

Copy link

vercel bot commented Jan 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Jan 18, 2024 2:44am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
docs-i18n ⬜️ Ignored (Inspect) Visit Preview Jan 18, 2024 2:44am

@astrobot-houston
Copy link
Contributor

Hello! Thank you for opening your first PR to Astro’s Docs! 🎉

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any broken links you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Netlify 🥳

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@sarah11918 sarah11918 added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Jan 8, 2024
@sarah11918
Copy link
Member

Hi @nadar! I'm glad that you found a solution that worked for you, and that you want to help others who have the same issue! Thank you for coming to add extra information to the docs.

We really try not to add "notes" and "tips" unless they fit certain criteria. These should be things that are almost not a part of docs at all, but extra, external or tangential things. If the information is truly important, then it should be properly documented as a part of the instructions or explanation.

For example, if it is necessary to also update another file, then we should properly show that file being updated as part of the complete instructions. This does not go in a "note" if it's key information. Would you be willing to revise this to include it at the appropriate step, not using a "note" for things that should have been done in the first place?

@nadar
Copy link
Contributor Author

nadar commented Jan 8, 2024

Thank you @sarah11918 for guiding me in the right direction. I've revised the sentence and eliminated the note syntax. Hopefully, it's better now. Since English is not my first language, I don't want to mess up the quality of the Astro documentation. 🙂

Copy link
Contributor

@VoxelMC VoxelMC 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 @sarah11918 for guiding me in the right direction. I've revised the sentence and eliminated the note syntax. Hopefully, it's better now. Since English is not my first language, I don't want to mess up the quality of the Astro documentation. 🙂

Hello! Thank you for taking on the task of ensuring no one makes this mistake in the future!

I have taken a stab at revising this in a way that is more consistent with the Astro docs at the moment.

I would also like to request someone in core to confirm that this is the correct solution, before moving forward!

I have a question about this, as I have never written an integration. Would

{
  "exports": "./dashboard.astro"
}

be the same as

{
  "exports": { "./dashboard.astro": "./dashboard.astro" }
}

?

src/content/docs/en/reference/integrations-reference.mdx Outdated Show resolved Hide resolved
@Fryuni
Copy link
Member

Fryuni commented Jan 14, 2024

Those are not the same @VoxelMC

"export": "./dashboard.astro" allows importing that page from the root of the component:

import Dashboard from '@some/integration';

It is equivalent to this:

"exports": {
  ".": "./dashboard.astro"
}

To import from a subpath like this

import Dashboard from '@some/integration/dashboard.astro';

Is what requires this export:

"exports": {
  "./dashboard.astro": "./dashboard.astro"
}

The same applied for entrypoints passed to injectRoute

More information here: https://nodejs.org/api/packages.html#subpath-exports

It's not an Astro requirement, Node won't even try to resolve the import if it is not declared in the package, so Vite's module loader won't be called, and neither is Astro's Vite plugin.

@VoxelMC
Copy link
Contributor

VoxelMC commented Jan 14, 2024

Those are not the same @VoxelMC

"export": "./dashboard.astro" allows importing that page from the root of the component:

import Dashboard from '@some/integration';

It is equivalent to this:

"exports": {
  ".": "./dashboard.astro"
}

To import from a subpath like this

import Dashboard from '@some/integration/dashboard.astro';

Is what requires this export:

"exports": {
  "./dashboard.astro": "./dashboard.astro"
}

The same applied for entrypoints passed to injectRoute

More information here: https://nodejs.org/api/packages.html#subpath-exports

It's not an Astro requirement, Node won't even try to resolve the import if it is not declared in the package, so Vite's module loader won't be called, and neither is Astro's Vite plugin.

Ohhhh, that makes a lot more sense. You explained it much better than the npm and nodejs docs, imo! Thanks Fryuni :)

@sarah11918
Copy link
Member

Would love a final review over this content from @nadar and @Fryuni ! (Especially since I proposed shortening the suggestion.)

What is the group consensus here? Do we think this is enough to be helpful in the docs??

@VoxelMC
Copy link
Contributor

VoxelMC commented Jan 18, 2024

Would love a final review over this content from @nadar and @Fryuni ! (Especially since I proposed shortening the suggestion.)

What is the group consensus here? Do we think this is enough to be helpful in the docs??

I think it is a pretty niche case. However, I can see it being a very annoying bug to research if it happens to someone. I agree with shortening the suggestion. I say keep it!

What do you say to it being a :::note aside? Probably no, but a thought!

@Fryuni
Copy link
Member

Fryuni commented Jan 18, 2024

I think the shortened version is enough, no need to get in the details of how it fails otherwise.

Users will see that on their terminal if they don't follow.

I'm a bit biased on whether the text is clear or not because I already know the context, answers and reasoning behind it. I'd feel better with an opinion from someone who never wrote an integration to see what they would do after reading that line.

But besides that LGTM

@sarah11918
Copy link
Member

Alright, I'm going to take this all as NWTWWHB!

(PS: Not a "note" because it's actually a required step. All the asides are for things outside of "normal procedure")

@sarah11918 sarah11918 added the Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)! label Jan 18, 2024
@sarah11918 sarah11918 merged commit f7dfaca into withastro:main Jan 18, 2024
8 checks passed
@nadar nadar deleted the patch-1 branch January 18, 2024 08:22
ematipico pushed a commit that referenced this pull request Jan 26, 2024
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Luiz Ferraz <[email protected]>
Co-authored-by: voxel!() <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve documentation Enhance existing documentation (e.g. add an example, improve description) Merge Queue Approved and ready to be merged (wait for feature release if also labelled M-O-R)!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants