fix(rollup-plugin): plugin now correctly resolves relative ts imports #1516
Conversation
| const normalizedPath = path.resolve(path.dirname(importer), importee); | ||
| const absPath = pluginUtils.addExtension(normalizedPath); | ||
| const absPath = pluginUtils.addExtension(normalizedPath, ext); |
There was a problem hiding this comment.
this is assuming that the importer dictates the importee's extension. What if you're specifying the extension explicitly?
There was a problem hiding this comment.
yea, I guess I don't usually see imports specifying the extension (import foo from ./foo.js) so didn't think of that... I will update to check if importee has extension specified then if not assume it's the same as importer (since most .ts files will import other .ts files and most .js files will import other .js files). Sound good?
| @@ -51,14 +51,15 @@ module.exports = function rollupLwcCompiler(pluginOptions = {}) { | |||
|
|
|||
| // Normalize relative import to absolute import | |||
| if (importee.startsWith('.') && importer) { | |||
| const ext = path.extname(importee) || path.extname(importer); | |||
There was a problem hiding this comment.
@wes566 why are you checking the extension of both importer and importee?
There was a problem hiding this comment.
So first it checks if the importee specifies an extensions... so if you had import foo from ./foo.js it would use .js... whereas if the import statement said import foo from ./foo then it will match the importer's extension.
There was a problem hiding this comment.
what happens if your entry file has a '.js' extension, however your imported file, by mistake, has a .ts extension?
// entry.js
import foo from './foo' // foo points to foo.ts
There was a problem hiding this comment.
Ok what we are saying is that if the file you are in is TS, by we will try to match it, but if your relative file is for whatever weird reason.
So yes we should throw
| import { api, LightningElement } from "lwc"; | ||
|
|
||
| export default class Foo extends LightningElement { | ||
| // @ts-ignore |
There was a problem hiding this comment.
why do we need the ignore?
There was a problem hiding this comment.
Without it my editor will complain "typescript does not support experimental decorators"... I could fix it by adding a tsconfig.json... but that feels like overkill for a simple test so it's easier to just ts-ignore this line
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
i would like to see a mixed case of js importing ts or vice versa to ensure the scenario is handled correctly
There was a problem hiding this comment.
We should actually throw if you are have a mix since it makes no sense or whatsoever.
There was a problem hiding this comment.
That would definitely simplify things... I wouldn't go as far as saying it makes "no sense" since mixing TS and JS is pretty common in brownfield projects that are trying to move to typescript slowly - after all, the typescript compiler has an option allowJs so it's very much a supported scenario from Typescripts point-of-view.
Having said that, mixing TS and JS is more of a feature to consider adding in the future if there is demand... the goal of this PR is to simply fix the bug of "my TS file cannot relatively import another TS file". So I like the idea for now of just throwing if TS and JS are mixed. I'll update the PR.
There was a problem hiding this comment.
If you are using LWC you are writing new things, so there is not upgrade path :) I think the logic is fine as it is but if you can throw when the missmatch happens it will avoid us to have to support that case all togeher,
|
@wes566 make sure we do throw if we detect a explicit missmatch between importer and importee extension and I will merge it. |
|
I think is your first PR, so congrats @wes566 ! and thank you |
Details
The rollup-plugin was failing to resolve relative imports if using Typescript. It was always assuming a
.jsfile extension. This PR fixes that bug and adds a test to ensure TS relative imports work.Does this PR introduce breaking changes?
No, it does not introduce breaking changes.The PR fulfills these requirements: