-
Notifications
You must be signed in to change notification settings - Fork 916
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
Improve import resolution logic #519
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/83srn5l89 |
const resolvedUrl = path.resolve(path.dirname(outPath), spec); | ||
allProxiedFiles.add(resolvedUrl); | ||
allProxiedFiles.add( | ||
resolvedImportUrl.startsWith('/') |
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.
bug fix: we weren't resolving /_dist_/
URLs properly, since they get handled as absolute paths by path.resolve
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.
👏
import srcFileExtensionMapping from './src-file-extension-mapping'; | ||
|
||
const cwd = process.cwd(); |
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.
how do you feel about adding cwd
to the config object? We do this in a few different places around the codebase, but it would be good to start moving towards a configurable cwd.
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 that’s a great idea.
0cc5e05
to
66aef73
Compare
styleEl.type = 'text/css'; | ||
|
||
styleEl.appendChild(codeEl); | ||
document.head.appendChild(styleEl); |
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! This test will really come in handy for some upcoming work.
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.
Nice! This is looking pretty clean to me.
66aef73
to
d7fa9e1
Compare
Resolves #439, Resolves #443
This PR improves our ability to resolve a partial import (like
src/components
orsrc/components/index
to importsrc/components/index.js
). It solves a couple of bugs that users had been reporting, where behavior didn't match Node or Webpack's resolution logic (see resolved issues above).