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

refactor(github-pages): add --dotfiles to deploy command hint #2158

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

Aareksio
Copy link
Contributor

@Aareksio Aareksio commented Feb 21, 2024

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Without --dotfiles gh-pages does not deploy .nojekyl
Ref: https://www.npmjs.com/package/gh-pages#optionsdotfiles

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Without `--dotfiles` `gh-pages` does not deploy `.nojekyl`
@pi0
Copy link
Member

pi0 commented Feb 21, 2024

Have you tried --nojekyll doesn't it adds the specific dotfile? Checking CLI:

  --nojekyll               Add a .nojekyll file to disable Jekyll

@Aareksio
Copy link
Contributor Author

Aareksio commented Feb 21, 2024

This should also work. Went with --dotfiles because the preset already creates .nojekyl file.

And I believe this is better, as users may decide to deploy without using 3rd party gh-pages utility.

@pi0
Copy link
Member

pi0 commented Feb 21, 2024

Main issue is that there might be dot files that are dangerous to be published. I think users they always have this choice and this is just a "hint" how to deploy. Sticking with safest is probably better while you might want to publish all dot files. (also the fact that gh pages CLI itself chose this default behavior)

@Aareksio
Copy link
Contributor Author

Aareksio commented Feb 21, 2024

Following the current hint leads to broken deployment.

I have considered four solutions:

  • (a) add --dotfiles, align the behavior with gh action from docs
  • (b) add --nojekyll, different mechanics, the same result
  • (c) remove hint altogether, there is no point in having a command "hint" if user is required to read 3rd party docs
  • (d) update the hint to something more complicated

The mentioned docs already suggest deploying .output/public without care for dotfiles. To my knowledge dotfiles have not been a consideration before in any preset, despite the risk being shared across all options.

a seems to be the least invasive. It aligns the hint with existing documentation.

b achieves the same result which is satisfactory for me. It may cause issues for users expecting their dot-prefixed paths (eg. .well-known) to be published, but I'd say these are fairly easy to debug.

@pi0
Copy link
Member

pi0 commented Feb 21, 2024

It is odd for GitHub pages that ignore .* by default we don't have such consideration for other providers because they also don't ignore dot files by default. (cloudflare pages ignores specific known ones for example)

But fair enough for other common dirs, it might came useful and hint is already clear what user is doing πŸ‘πŸΌ

@pi0 pi0 changed the title fix(github-pages): add necessary --dotfiles to deploy commands refactor(github-pages): add --dotfiles to deploy command hint Feb 21, 2024
@Aareksio
Copy link
Contributor Author

gh-pages is not official package. I think ignoring dotfiles is not a conscious design decision. This is the glob matcher used underneath default behavior. The option dates back at least 11 years and seems to be added in response to need to include dotfiles, without introducing breaking change.

@pi0 pi0 merged commit 1b31b07 into nitrojs:main Feb 27, 2024
@pi0 pi0 mentioned this pull request Feb 27, 2024
@Aareksio Aareksio deleted the patch-1 branch November 7, 2024 18:55
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