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(config): fix dynamic directory issue on windows #519

Closed
wants to merge 1 commit into from
Closed

fix(config): fix dynamic directory issue on windows #519

wants to merge 1 commit into from

Conversation

jonaseriksson84
Copy link

@jonaseriksson84 jonaseriksson84 commented Mar 17, 2023

What is the nature of this change?

  • New feature submission
  • Routine bug fix
  • Site/documentation improvement
  • Demo code improvement
  • Component style/interaction improvement
  • TypeScript definition update
  • Package size optimization
  • Performance optimization
  • Function enhancement
  • Internationalization improvement
  • Refactoring
  • Code style optimization
  • Test cases
  • Branch merging
  • Other changes (What are the changes about?)

🔗 Related Issue
Related issue
fix #516

💡 Background and solution of the requirement
When doing npm run now-build a directory named like this is created:
C:\<project>\node_modules\rc-pagination\dist\~demos\:id
This (: in path) is not allowed on Windows, and fails. Same problem happens with Bazel - see this issue for example.
When explicitly setting exportStatic to false in the config file, this file is not produced, so that should take care of this issue.

Note, I can't read any kind of Chinese so I don't know for sure what the documentation says about this option:
https://v1.d.umijs.org/config#exportstatic

📝 Update log
Language Update Description

  • 🇺🇸 English
  • 🇨🇳 Chinese

☑️ Self-checklist before requesting a merge
⚠️ Please self-check and tick all options. ⚠️

Documentation is supplemented or not required

  • Code demo is provided or not required
  • TypeScript definition is supplemented or not required
  • Changelog is provided or not required

@vercel
Copy link

vercel bot commented Mar 17, 2023

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

Name Status Preview Comments Updated
pagination ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 17, 2023 at 8:40AM (UTC)

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #519 (305d18a) into master (e27a1c1) will not change coverage.
The diff coverage is n/a.

❗ Current head 305d18a differs from pull request most recent head 802bc33. Consider uploading reports for the commit 802bc33 to get more accurate results

@@           Coverage Diff           @@
##           master     #519   +/-   ##
=======================================
  Coverage   92.87%   92.87%           
=======================================
  Files           4        4           
  Lines         351      351           
  Branches      125      125           
=======================================
  Hits          326      326           
  Misses         25       25           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@PeachScript PeachScript left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! But this project is now using dumi 2, and dumi 2 is already exclude dynamic directory for windows, ref

So can you provide detailed errors for your local project if the problem still existing?

@jonaseriksson84
Copy link
Author

Thanks for your contribution! But this project is now using dumi 2, and dumi 2 is already exclude dynamic directory for windows, ref

So can you provide detailed errors for your local project if the problem still existing?

Ah ok, this breaks for us on linux when we're using Bazel as mentioned in the PR above. The path with the : would still be produced, but Bazel would refuse to handle that file. I guess I should raise that issue in Dumi instead.

However, I think @yoyo837 's PR #520 does it more correctly as it doesn't make sense to include dist in the built project, so this one can be closed in favor of that PR.

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.

Error while installing with yarn
3 participants