Skip to content

Fix the conversion of private names to public names #275

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix the conversion of private names to public names #275

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23d0b2b
Fix private variable detection to not match multiple dash
Tofandel Apr 8, 2025
ebb3b65
Make _isPrivate match Sass semantics
nex3 Apr 8, 2025
6deefe5
Properly deprivatize names that start with multiple underscores
nex3 Apr 8, 2025
13f065d
Fix prefix removal without a trailing dash
nex3 Apr 9, 2025
00940e2
Code review
nex3 Apr 10, 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
55 changes: 42 additions & 13 deletions lib/src/migrators/module.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import 'package:args/args.dart';
import 'package:collection/collection.dart';
import 'package:charcode/charcode.dart';
import 'package:path/path.dart' as p;
import 'package:sass_api/sass_api.dart';
import 'package:source_span/source_span.dart';
Expand Down Expand Up @@ -191,6 +192,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
"Can't access _configuredVariables when not visiting a dependency."));
Set<MemberDeclaration<VariableDeclaration>>? __configuredVariables;

/// The number of variables that have been generated from whole cloth.
var _generatedVariables = 0;

/// A mapping between member declarations and references.
///
/// This performs an initial pass to determine how a declaration seen in the
Expand Down Expand Up @@ -263,7 +267,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
var forwardsByUrl = <Uri, Map<String, Set<MemberDeclaration>>>{};
var hiddenByUrl = <Uri, Set<MemberDeclaration>>{};
for (var declaration in references.globalDeclarations) {
var private = declaration.name.startsWith('-');
var private = _isPrivate(declaration.name);

// Whether this member will be exposed by the regular entrypoint.
var visibleAtEntrypoint = !private &&
Expand Down Expand Up @@ -352,11 +356,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
if (declaration.isForwarded) return;

var name = declaration.name;
if (name.startsWith('-') &&
if (_isPrivate(name) &&
references.referencedOutsideDeclaringStylesheet(declaration)) {
// Remove leading `-` since private members can't be accessed outside
// the module they're declared in.
name = name.substring(1);
// Private members can't be accessed outside the module they're declared
// in.
name = _privateToPublic(name);
}
name = _unprefix(name);
if (name != declaration.name) {
Expand All @@ -365,6 +369,27 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
}
}

/// Returns whether [identifier] is a private member name.
///
/// Assumes [identifier] is a valid CSS identifier.
bool _isPrivate(String identifier) {
return identifier.startsWith('-') || identifier.startsWith('_');
}

/// Converts a private identifier to a public one.
String _privateToPublic(String identifier) {
assert(_isPrivate(identifier));
for (var i = 0; i < identifier.length; i++) {
Copy link
Contributor

@pamelalozano16 pamelalozano16 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why are you iterating through the identifier? Shouldn't returning the substring without the initial - or _ should be enough?

Copy link

@Tofandel Tofandel Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There can be multiple dashes or underscore, removing only one if there is multiple would lead to the variable still being private, see linked issue.

It's just iterating each char until it finds a character that's not an underscore or dash, then returns the string without the iterated part

This is more performant than regex

var char = identifier.codeUnitAt(i);
if (char != $dash && char != $underscore) {
return identifier.substring(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about identifier.replace(Regex(r"^-|^_"),"") ?

Copy link

Choose a reason for hiding this comment

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

The correct one would be identifier.replace(Regex(r"^[-_]+"),"")

Copy link

Choose a reason for hiding this comment

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

And if the returned string is empty then return a generated var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I habitually avoid regular expressions because they're somewhat more expensive than explicit parsing. If you'd prefer it, I can make the change, but I think the current way does work.

}
}

_generatedVariables++;
return 'var${_generatedVariables}';
}

/// Returns whether the member named [name] should be forwarded in the
/// entrypoint.
///
Expand Down Expand Up @@ -1025,8 +1050,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
newName = declaration.name.substring(importOnlyPrefix.length);
}

if (_shouldForward(declaration.name) &&
!declaration.name.startsWith('-')) {
if (_shouldForward(declaration.name) && !_isPrivate(declaration.name)) {
var subprefix = "";
if (importOnlyPrefix != null) {
var prefix = _prefixFor(declaration.name);
Expand All @@ -1036,7 +1060,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
}
if (declaration.name != newName) _needsImportOnly = true;
shownByPrefix.putIfAbsent(subprefix, () => {}).add(declaration);
} else if (!newName.startsWith('-')) {
} else if (!_isPrivate(newName)) {
hidden.add(declaration);
}
}
Expand Down Expand Up @@ -1076,7 +1100,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
if (declaration is ImportOnlyMemberDeclaration) {
name = name.substring(declaration.importOnlyPrefix.length);
}
if (name.startsWith('-')) name = name.substring(1);
if (_isPrivate(name)) name = _privateToPublic(name);
name = _unprefix(name);
if (subprefix.isNotEmpty) name = '$subprefix$name';
if (declaration.member is VariableDeclaration) name = '\$$name';
Expand Down Expand Up @@ -1205,7 +1229,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
void _renameReference(FileSpan span, MemberDeclaration declaration) {
var newName = renamedMembers[declaration];
if (newName != null) {
if (newName.startsWith('-') &&
if (_isPrivate(newName) &&
declaration.name.endsWith(newName.substring(1))) {
addPatch(patchDelete(span,
start: 1, end: declaration.name.length - newName.length + 1));
Expand All @@ -1228,11 +1252,16 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
///
/// Otherwise, returns [name] unaltered.
String _unprefix(String name) {
var isPrivate = name.startsWith('-');
var unprivateName = isPrivate ? name.substring(1) : name;
var isPrivate = _isPrivate(name);
var unprivateName = isPrivate ? _privateToPublic(name) : name;
var prefix = _prefixFor(unprivateName);
if (prefix == null) return name;
return (isPrivate ? '-' : '') + unprivateName.substring(prefix.length);

var withoutPrefix = unprivateName.substring(prefix.length);
if (_isPrivate(withoutPrefix)) {
withoutPrefix = _privateToPublic(withoutPrefix);
Copy link

Choose a reason for hiding this comment

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

In this case maybe it's better to only run substring(1), otherwise multiple dashes will get replaced to 1 dash still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? _privateToPublic() should handle multiple dashes gracefully.

}
return (isPrivate ? '-' : '') + withoutPrefix;
Copy link

@Tofandel Tofandel Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
return (isPrivate ? '-' : '') + withoutPrefix;
return (isPrivate ? name.substring(0, 1) : '') + withoutPrefix;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally I'm okay with leaning towards encouraging -, since that's definitely the more CSS-style prefix.

Copy link

@Tofandel Tofandel Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it's just for the sake of avoiding unecessary changes during a migration, but I definitely don't understand everything that's happening after this runs, because some tests that look like they should fail returning a dash, don't 🤷

<==> arguments
--migrate-deps --remove-prefix=app
<==> input/entrypoint.scss
@import "library";
a {
  color: $app-var;
}

<==> input/_library.scss
$_app-red: red;
$app-var: $_app-red;

<==> output/entrypoint.scss
@use "library";
a {
  color: library.$var;
}

<==> output/_library.scss
$_red: red;
$var: $_red;

Ok, that one fails

<==> arguments
--migrate-deps --remove-prefix=app
<==> input/entrypoint.scss
@import "library";
a {
  color: $app-var;
}

<==> input/_library.scss
$__app-red: red;
$app-var: $__app-red;

<==> output/entrypoint.scss
@use "library";
a {
  color: library.$var;
}

<==> output/_library.scss
$__red: red;
$var: $__red;

So now it's just a question of do we want to preserve the original number of dashes/underscore or not? (My previous implementation did)

}

/// Returns the namespace that built-in module [module] is loaded under.
Expand Down
39 changes: 39 additions & 0 deletions test/migrators/module/remove_prefix_flag/without_dash.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<==> arguments
--remove-prefix=lib

<==> input/entrypoint.scss
$lib-a: 5;
$lib_b: $lib-a + 2;

@function lib-fn($lib-local) {
@return $lib-local;
}

@mixin lib-mixin {
lib-property: lib-value;
}

lib-selector {
lib-property: lib-fn(0);
@include lib-mixin;
}

<==> output/entrypoint.scss
$a: 5;
$b: $a + 2;

@function fn($lib-local) {
@return $lib-local;
}

@mixin mixin {
lib-property: lib-value;
}

lib-selector {
lib-property: fn(0);
@include mixin;
}

<==> output/entrypoint.import.scss
@forward "entrypoint" as lib-*;
25 changes: 0 additions & 25 deletions test/migrators/module/scope/pseudoprivate.hrx
View file
Open in desktop

This file was deleted.

19 changes: 19 additions & 0 deletions test/migrators/module/scope/pseudoprivate/dash.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<==> arguments
--migrate-deps
<==> input/entrypoint.scss
@import "library";
a {
color: $-var;
}

<==> input/_library.scss
$-var: red;

<==> output/entrypoint.scss
@use "library";
a {
color: library.$var;
}

<==> output/_library.scss
$var: red;
19 changes: 19 additions & 0 deletions test/migrators/module/scope/pseudoprivate/dashes_only.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<==> arguments
--migrate-deps
<==> input/entrypoint.scss
@import "library";
a {
color: $---;
}

<==> input/_library.scss
$---: red;

<==> output/entrypoint.scss
@use "library";
a {
color: library.$var1;
}

<==> output/_library.scss
$var1: red;
19 changes: 19 additions & 0 deletions test/migrators/module/scope/pseudoprivate/double_dash.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<==> arguments
--migrate-deps
<==> input/entrypoint.scss
@import "library";
a {
color: $--var;
}

<==> input/_library.scss
$--var: red;

<==> output/entrypoint.scss
@use "library";
a {
color: library.$var;
}

<==> output/_library.scss
$var: red;
21 changes: 21 additions & 0 deletions test/migrators/module/scope/pseudoprivate/indirect.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<==> arguments
--migrate-deps
<==> input/entrypoint.scss
@import "library";
a {
color: $middle;
}

<==> input/_library.scss
$-upstream: red;
$middle: $-upstream;

<==> output/entrypoint.scss
@use "library";
a {
color: library.$middle;
}

<==> output/_library.scss
$-upstream: red;
$middle: $-upstream;
19 changes: 19 additions & 0 deletions test/migrators/module/scope/pseudoprivate/underscore.hrx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<==> arguments
--migrate-deps
<==> input/entrypoint.scss
@import "library";
a {
color: $_var;
}

<==> input/_library.scss
$_var: red;

<==> output/entrypoint.scss
@use "library";
a {
color: library.$var;
}

<==> output/_library.scss
$var: red;