-
Notifications
You must be signed in to change notification settings - Fork 973
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(web): Fix importing .ts/.tsx files in web with directory-named-import plugin #1332
Conversation
@@ -9,7 +10,16 @@ const getNewPath = (value: string, filename: string) => { | |||
const newImportPath = [dirname, basename, basename].join('/') | |||
|
|||
try { |
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.
Since resolveFile
returns null when the path isn't found, I think we can remove the try/catch blocks.
try { | |
return resolveFile(path.join(path.dirname(filename), newImportPath)) |
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.
👍
oooh, 🦜🦆 with 🗻 |
Note: there is one failing test EDIT: done |
Oh windows! |
I think @Tobbe contributed an utility called |
Giving that a go now 👍 |
44e11ec
to
cf087ad
Compare
@peterp ready for review. Finally got them windows paths! ✅ |
@@ -35,7 +43,7 @@ export default function ({ types: t }: { types: typeof types }): PluginObj { | |||
|
|||
const newPath = getNewPath(value, <string>filename) | |||
if (!newPath) return | |||
const newSource = t.stringLiteral(value.replace(value, newPath)) | |||
const newSource = t.stringLiteral(ensurePosixPath(newPath)) |
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.
<3
{ | ||
input: 'import pew from "./__fixtures__/directory-named-imports/Module"', | ||
output: | ||
'import pew from "./__fixtures__/directory-named-imports/Module/Module"', | ||
output: `import pew from "${POSIX_DIRNAME}/__fixtures__/directory-named-imports/Module/Module.js";`, |
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.
Had a thought. Something feels off here, because imports in NodeJS are always in unix style. I guess it's because of the way that we get the absolute paths.
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.
Not exactly happy with this. There's nothing actionable, just thought I would mention it.
THanks <3! |
As title :)
Resolves #1290
Resolves #1214
todo