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.

Sign up for GitHub

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

Jump to bottom

fix: use cwd + nue root as base for relative file output path #232

Merged
merged 5 commits into from
Mar 25, 2024
Merged

fix: use cwd + nue root as base for relative file output path #232

merged 5 commits into from
Mar 25, 2024

Conversation

nobkd
Copy link
Collaborator

@nobkd nobkd commented Mar 14, 2024 •
edited
Loading

Allows to set paths relative to the root dir passed to the nue command instead of the cwd.
Also enables setting absolute paths

  • Fix scripts not being copied/minified to the output folder 🤷
  • Tests, etc.
  • There will always be something missing on some to-do list :)

Will have to investigate, why no js files are being copied.

EDIT: Ubuntu and Mac shouldn't be successful, as the paths are different from before...
Only Windows is correct with its test results...

@nobkd nobkd changed the title fix: use root as base if not abs path fix: use nue root as base if relative path instead of cwd Mar 14, 2024
@tipiirai
Copy link
Contributor

Are you trying to get rid of the this chdir command (which bothers me) or is this something else?

@tipiirai
Copy link
Contributor

Ahh... sorry. I did not concentrate enough. Looks like this relates to the root command line argument. Nice, makes sense. Is there a bug or the code is ugly?

@nobkd
Copy link
Collaborator Author

nobkd commented Mar 15, 2024 •
edited
Loading

The bug is, that you have to locate your output dir relative to the position of the place where the nue command is executed and not relative to the root, the nue command has been given

Edit: This is a bug, because it is unexpected, that the path is not located relative to the site.yaml, where you change the output path.

@nobkd nobkd changed the title fix: use nue root as base if relative path instead of cwd fix: use cwd + nue root as base for relative file output path Mar 15, 2024
@nobkd
Copy link
Collaborator Author

nobkd commented Mar 22, 2024 •
edited
Loading

Still WIP. I need to do some local testing and maybe add a test case too.

Also, I'm not sure, if the tests should really pass currently...

@nobkd
Copy link
Collaborator Author

nobkd commented Mar 24, 2024 •
edited
Loading

Ok, fixed the tests. They shouldn't have passed on GitHub before... 🤷
Maybe @fritx does have an idea why they did, even when they shouldn't have?

I think this is ready for review. I'm not that happy with the name of the helper function joinRootPath, so if you have a better name, let me know.

I think will rebase #230 on this, to simplify the logic for the following lines with joinRootPath: https://github.com/nuejs/nue/pull/230/files#diff-d08a333705c3aee86e4360625e0aa4e9a8be4ae5c86cbe642d0deecaf38c1868R83-R84

@nobkd nobkd marked this pull request as ready for review March 24, 2024 03:02
@nobkd nobkd requested review from tipiirai and fritx March 24, 2024 03:03
@nobkd nobkd mentioned this pull request Mar 24, 2024
Closed
2 tasks
@tipiirai tipiirai merged commit 850e85f into nuejs:master Mar 25, 2024
3 checks passed
@tipiirai
Copy link
Contributor

Looks solid. Thank you!

@nobkd nobkd deleted the fix/add-cwd-to-root-pth branch March 25, 2024 06:56
fritx added a commit to fritx/nue that referenced this pull request Apr 7, 2024
@fritx
Fix some tests not running (mentioned in nuejs#232), removing process…
7353547 
….exit (added in nuejs#237) instead of terminate kit.serve properly while testing
fritx added a commit to fritx/nue that referenced this pull request Apr 7, 2024
@fritx
Fix missing toMatchPath for Windows testing (missing in nuejs#232)
60622bf
@fritx fritx mentioned this pull request Apr 7, 2024
Merged
fritx added a commit to fritx/nue that referenced this pull request Apr 7, 2024
@fritx
Fix some tests not running (mentioned in nuejs#232), removing process…
7bbe5be 
….exit (added in nuejs#237) instead of terminate kit.serve properly while testing
fritx added a commit to fritx/nue that referenced this pull request Apr 7, 2024
@fritx
Fix missing toMatchPath for Windows testing (missing in nuejs#232)
1983be4
@fritx
Copy link
Collaborator

fritx commented Apr 7, 2024

Ok, fixed the tests. They shouldn't have passed on GitHub before... 🤷
Maybe @fritx does have an idea why they did, even when they shouldn't have?

Looks like there are bugs in tests -- some tests are not running, due to a process.exit().
I've figured it out while working on another PR...
Submitting a PR -- #254

tipiirai pushed a commit that referenced this pull request Apr 20, 2024
@fritx @tipiirai
Fix some tests not running (mentioned in #232), removing process.exit…
fb0ea09 
… (added in #237) instead of terminate kit.serve properly while testing
tipiirai pushed a commit that referenced this pull request Apr 20, 2024
@fritx @tipiirai
Fix missing toMatchPath for Windows testing (missing in #232)
5490a15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tipiirai tipiirai Awaiting requested review from tipiirai

@fritx fritx Awaiting requested review from fritx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nobkd @tipiirai @fritx