Skip to content
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.

Sign up for GitHub

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 a component namespace and name parsing issue #5117

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix a component namespace and name parsing issue #5117

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba87df0
Fix a component namespace and name parsing issue
Mr-VincentW Jan 9, 2025
d62ce4c
Simplified the fix
Mr-VincentW Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@lwc/rollup-plugin/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Comment on lines 333 to 334
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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`
// Note we do not need to use path.sep with `specifier` because it contains
// a '/' regardless of Windows vs Unix, since it comes from the Rollup `id`
// We do use path.sep with `filename` because that is not normalized

This comment should be updated, because now we do use path.sep, but not always.

specifier?.split('/') ?? path.dirname(filename).split('/').slice(-2);
specifier?.split('/') ?? path.dirname(filename).split(/[\\/]/).slice(-2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
specifier?.split('/') ?? path.dirname(filename).split(/[\\/]/).slice(-2);
specifier?.split('/') ?? path.dirname(filename).split(path.sep).slice(-2);

Are there any cases where we'd want to split on / on windows or \ on unix?

Copy link
Contributor

@cardoso cardoso Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that just reverts #5038 🫠 so it would be a regression. path.sep is very unreliable.

Copy link
Author

@Mr-VincentW Mr-VincentW Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the filename is extracted from the parameter id which could be a Rollup-normalized id, a hard-coded string (e.g., @lwc/resources/empty_css.css) or something generated by the custom resolveId hook, it could use either \ or / as the path delimiters on Windows. Therefore path.sep will not work as expected here, I reckon?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, path.sep won't work. I'm not sure what the right fix is. I haven't used Windows since XP.


/* v8 ignore next */
if (!namespace || !name) {
Expand Down