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.

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

Support --forward=prefixed across migrator runs #124

Merged
merged 4 commits into from Nov 5, 2019

Conversation

@jathak
Copy link
Member

jathak commented Nov 4, 2019

Fixes #123.

If a member was unprefixed by a previous migrator run, running the
migrator on a downstream stylesheet with --forward=prefixed should
still forward that member.

Fixes #123.

If a member was unprefixed by a previous migrator run, running the
migrator on a downstream stylesheet with `--forward=prefixed` should
still forward that member.
@jathak jathak requested a review from nex3 Nov 4, 2019
Copy link
Contributor

nex3 left a comment

Can you add a test for importing a migrated library without --remove-prefix, and one with --remove-prefix for a different prefix than the library itself used?

@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 4, 2019

Added a test with a different prefix. --forward=prefixed is not allowed without --remove-prefix, so I don't think a test is necessary for that.

@jathak jathak requested a review from nex3 Nov 4, 2019
Copy link
Contributor

nex3 left a comment

Ah, good point. What about --premove-prefix where the prefix is itself a subprefix of the old prefix? For example:

<==> arguments
--migrate-deps --remove-prefix=lib- --forward=prefixed

<==> input/entrypoint.scss
@import "button";

a {
  color: $lib-button-variable;
}

<==> input/_button.scss
$variable: green;

<==> input/_button.import.scss
@forward "button" as lib-button-*;
@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 4, 2019

Oh, yeah, I forgot about cases like that. Would this be the best output?

<==> output/entrypoint.scss
@use "button";

@forward "button" as button-*;

a {
  color: button.$variable;
}

<==> output/entrypoint.import.scss
@forward "entrypoint" as lib-*;
@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 4, 2019

I think so, yeah.

@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 4, 2019

Okay, this should now support the subprefix case.

@jathak jathak requested a review from nex3 Nov 4, 2019
Tuple2<String, bool> _makeForwardRule(Uri url) {
var shown = <String>{};
Tuple2<List<String>, bool> _makeForwardRule(Uri url) {
var shownByPrefix = <String, Set<String>>{};
var hidden = <String>{};

// Divide all global members from dependencies into sets based on whether

This comment has been minimized.

Copy link
@nex3

nex3 Nov 5, 2019

Contributor

Consider updating this comment to explain the prefix-based division you're now also doing.

This comment has been minimized.

Copy link
@jathak

jathak Nov 5, 2019

Author Member

Done

importOnlyPrefix != null) {
subprefix = importOnlyPrefix.substring(prefixToRemove.length);
}
if (declaration.name.length != newName.length) _needsImportOnly = true;

This comment has been minimized.

Copy link
@nex3

nex3 Nov 5, 2019

Contributor

Why not declaration.name != newName?

This comment has been minimized.

Copy link
@jathak

jathak Nov 5, 2019

Author Member

Fixed. I was worried about an issue with underscores and dashes, but I forgot that both of these names should already be normalized to dashes.

: '$subprefix$item'
];
}
if (allHidden.isNotEmpty) forward += ' hide ${allHidden.join(", ")}';

This comment has been minimized.

Copy link
@nex3

nex3 Nov 5, 2019

Contributor

This generation logic is complex... are there test cases for generating multiple forwards with different hides and prefixes?

This comment has been minimized.

Copy link
@jathak

jathak Nov 5, 2019

Author Member

Added a test for different subprefixes and fixed a bug in References that it revealed.

References should now take prefixes into account when checking a
member's visibility in a `@forward` rule.
@jathak jathak requested a review from nex3 Nov 5, 2019
@nex3
nex3 approved these changes Nov 5, 2019
@jathak jathak merged commit aa01dff into master Nov 5, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jathak jathak deleted the already-unprefixed branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.