-
Notifications
You must be signed in to change notification settings - Fork 156
fix: fs.readFile emit relative assets using cwd #542
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
Conversation
src/analyze.ts
Outdated
typeof staticChildValue.value === 'string' && | ||
staticChildValue.value.startsWith('.') | ||
) { | ||
staticChildValue.value = path.resolve(dir, staticChildValue.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we matching behavior here? vercel/next.js#84155 seems like this is diverging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm - yeah, I have it wrong here, I should use cwd
- and update the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have:
├── hello.json
├── index.js
├── lib
│ └── index.js
└── package.json
With lib/index.js
as:
const fs = require("fs/promises")
fs.readFile('./hello.json', 'utf-8').then(console.log)
And index.js
as:
require("./lib")
Then node index.js
resolves hello.json in lib from the cwd
, not the lib/index.js
file. Also follows that node lib/index.js
resolves from the root too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this branch, running npm run build && node out/cli.js print test/unit/fs-emission/input.js
won't output asset1.txt
cuz it doesn't exist in the root.
If we place a dummy asset1.txt
at the root and re-run, then we trace the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have two variations one with cwd
at the root and once in the fixture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, both tests pass with that condition - I shared that as an example of how to test manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks! 🥇
🎉 This PR is included in version 0.30.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In this input file:
We don't emit for
fs.readFile('./asset1.txt')
, but we should: