Fix a component namespace and name parsing issue #5117
Conversation
The original approach cannot correctly parse `namespace` and `name` from file path in Windows as the path may contain `\` as delimeters. It doesn't work well for relative paths start with './' either which derives from implicit reference to CSS files.
|
Thanks for the contribution! Before we can merge this, we need @Mr-VincentW to sign the Salesforce Inc. Contributor License Agreement. |
There was a problem hiding this comment.
Thank you for looking into this!
In the original issue you mentioned:
Using /[\/]/ here solves the problem for me.
Can you simplify this PR to fix just that and provide a minimal repo that demonstrates this issue?
I gather you're trying to solve what you also mentioned:
It doesn't work well for relative paths start with ./ either which derives from implicit reference to CSS files from HTML templates.
I think this should be done as a separate PR once this one is merged.
--
PS: I'm only a contributor, not a maintainer, so this is just my advice based on a past windows-related issue and how tricky it can be to diagnose, validate the fix and ensure no breaking changes:
cardoso/vite-plugin-lwc#1 -> danjunger/vite-repro#5 -> #4825 (comment) -> #5038
| @@ -332,7 +332,7 @@ export default function lwc(pluginOptions: RollupLwcOptions = {}): Plugin { | |||
| const [namespace, name] = | |||
| // Note we do not need to use path.sep here because this filename contains | |||
| // a '/' regardless of Windows vs Unix, since it comes from the Rollup `id` | |||
| specifier?.split('/') ?? path.dirname(filename).split('/').slice(-2); | |||
| specifier?.split('/') ?? /(?:([^\\/]+?)[\\/])?([^\\/]+?)(?:[\\/]\2)?(?:\.scoped)?\.[^.]+$/.exec(filename)?.slice(1); | |||
There was a problem hiding this comment.
I don't think using a regex here is the right approach.
However, in my case (on Windows), I found them cometimes containing \ as well.
Are you able to debug what specifier and filename are here in this case you mentioned?
Ideally this line remains unchanged, and the filename is normalized via simple String.replace previous to it.
The assumption is that rollup already does this normalization, which is why providing a log of the variables as well a minimal repo would be needed to figure out the proper fix.
There was a problem hiding this comment.
I've updated my fix to only fix the delimiter issue.
BTW, rollup does normalize the ids but not the importer, and the LWC rollup plugin has a custom resolveId hook in the plugin to resolves ids based on both importee (id) and importer, which is where the issue derives.
There was a problem hiding this comment.
Here's a screenshot of what the specifire and filename look like:
The specifier is always null for "directory modules" as it's parsed from a URL search param which is only appended to "alias modules" by the resolveId hook.
Simplified the fix as only having replaced the path delimiter for parsing `namespace` and `name` from filenames.
Hey, thank you for the feedback. I've simplified my fix and created a demo repo to showcase the issue. Now the fix only solves the path delimiter issue but doesn't handle the relative path problem. Cheers. |
|
@Mr-VincentW thanks a lot for the demo repo! Does the error still happen if you remove the html plugin and create the index.html file yourself? Example. If the error still happens, I suggest further simplifying the repo by removing the html plugin. If the error goes away, we need to understand why and figure out the best way to deal with it. |
|
@cardoso Yes, the issue continues. I tend not to remove the plugin in the repo, or you'll need to either copy the Anyway, it has nothing to do with the html. No matter if the demo can run or not in the browser, the issue happens when the package is built. Also, I have already explained the reason why this issue happens here. |
As per issue #5113 :
The original approach cannot correctly parse
namespaceandnamefrom file path in Windows as the path may contain\as delimeters.It doesn't work well for relative paths start with
./either which derives from implicit reference to CSS files from HTML templates.The fix still cannot extract the
namespacefor relative paths since there's only a./. However, it does correctly identify thenamefor the component.Details
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?