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

Better handling of added @use and @forward rules. #101

Merged
merged 6 commits into from Oct 10, 2019

Conversation

@jathak
Copy link
Member

commented Oct 1, 2019

Resolves #46.
Resolves #88.

@use and @forward rules should now follow the order discussed in #46, with the built-in module @use rules inserted immediately before the first existing import and the additional @use and @forward rules inserted immediately after the last existing import.

Code changes to make this happen:

  • Instead of one additionalUseRules sets, this is now split into three sets for built-in modules, load paths, and relative paths.
  • With this change, there are now eight different properties that are reset for each stylesheet in _ModuleMigrationVisitor.visitStylesheet. To reduce the boilerplate in this method, I extracted them out into a separate class so they can all be reset at once.
  • The text of a Patch is now mutable to allow an @import->@use patch to be changed to an @import->@forward patch without changing the patches' order (trying to track these patches separately and then add them later caused a lot of issues, mostly with @import rules with multiple imports)
  • I replaced getMigratedContents in MigrationVisitor with beforePatch, since we now needed to add patches after visiting the stylesheet but before patches and currentUrl are reset for the next stylesheet. addPatch now has an additional optional parameter to add patches to the beginning of the list, to resolve similar issues to the above with multiple patches at the same insertion point.
@jathak jathak requested a review from nex3 Oct 1, 2019
@jathak jathak force-pushed the use-order branch from 74a353b to 8632c7a Oct 1, 2019
Copy link
Contributor

left a comment

Can you add a test case for file that has everything at once: a new built-in @uses, an @imports converted to @use, an @import converted to @forward, a new relative @use, a new load-path @use, and a new relative @forward, and a new load-path @forward?

/// A migrator should override this if it needs to add any additional patches
/// after a stylesheet is visited.
@protected
void afterVisitingStylesheet(Stylesheet node) {}

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

I would call this beforePatch(), and rather than explaining what the state of the instance variables are (overriders should really be using node.span.sourceUrl over currentUrl anyway), say that it "is called before patches are applied".

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Done

lib/src/migrators/module.dart Outdated Show resolved Hide resolved
lib/src/migrators/module.dart Outdated Show resolved Hide resolved
/// at the end.
Set<String> _additionalUseRules;
/// Stores state for the migration of the current stylesheet.
StylesheetState _state;

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

I see why this is attractive, but it's a pattern I've avoided in the Sass implementation. I find that the layer of indirection adds more complexity than it solves by taking away a bunch of instance variables.

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Changed this back.

lib/src/patch.dart Outdated Show resolved Hide resolved
var afterSemicolon =
extendForward(extendThroughWhitespace(node.span), ';')?.end;
_state.afterLastImport = currentUrl.path.endsWith('.sass')
? node.span.end
: afterSemicolon ?? node.span.end;
Comment for lines 690  – 694

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

It's a little confusing that afterSemicolon is computed even when it's unnecessary. I'd probably just make this an if statement to avoid that.

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Done

/// When determining `@forward` rules to add based on --forward, if a URL that
/// would have an extra rule is a key in this map, the migrator will instead
/// mutate the patch to be a `@forward` rule instead of a `@use` rule.
final _possibleForwardConversions = <Uri, Patch>{};

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

I don't like storing patches to modify later... I hate to make such a nice self-contained class mutable, and it would be nice to keep the patch list as a write-only data structure as much as possible. To that end, would it be possible to determine in _migrateImport() whether a @use or a @forward should be generated, and just choose the correct one there and then?

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Yeah, I didn't think it was possible at first, but ultimately realized it was, so existing the new rule for existing @imports is now determined in _migrateImport.

lib/src/migrators/module.dart Outdated Show resolved Hide resolved
entry.value.sourceUrl == url) {
return true;
}
}

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

Could you simplify this by defining Set<SassNode> allReferences(MemberDeclaration declaration) in References, and then iterating over references.allDeclarations rather than repeating a bunch of logic for each reference type? That helper would probably also let you simplify References.referencedOutsideDeclaringStylesheet(), and potentially other logic that wants to do the same thing for all members of all types.

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Done. I also changed this to be a method of References.

<==> arguments
--migrate-deps

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

a {
color: mix($color, red);
}

<==> input/_color.scss
$color: blue;

<==> output/entrypoint.scss
@use "sass:color";
@use "color" as color2;

a {
color: color.mix(color2.$color, red);
}
Comment for lines -1  – -20

This comment has been minimized.

Copy link
@nex3

nex3 Oct 2, 2019

Contributor

Did you mean to delete this?

This comment has been minimized.

Copy link
@jathak

jathak Oct 3, 2019

Author Member

Yes. It was a duplicate of conflicting_namespaces_builtin.hrx (I renamed it in a previous PR, but accidentally only checked in the new file).

jathak added 2 commits Oct 1, 2019
Resolves #88.

If an `@import` rule's members are never referenced in the entrypoint
and that stylesheet is forwarded based on --forward, the `@forward` rule
will replace the `@import` instead of a `@use` rule.
@jathak jathak force-pushed the use-order branch from 8632c7a to 0af8564 Oct 2, 2019
@jathak jathak requested a review from nex3 Oct 3, 2019
@nex3
nex3 approved these changes Oct 10, 2019
var extras = useRulesToString(_additionalLoadPathUseRules) +
useRulesToString(_additionalRelativeUseRules) +
_getAdditionalForwardRules();
if (extras != '') {

This comment has been minimized.

Copy link
@nex3

nex3 Oct 10, 2019

Contributor
Suggested change
if (extras != '') {
if (extras == '') return;
@@ -48,4 +48,7 @@ class Patch {
buffer.write(file.getText(offset));
return buffer.toString();
}

toString() =>
"'${selection.text}' (${selection.start.offset}-${selection.end.offset}) -> '$replacement'";

This comment has been minimized.

Copy link
@nex3

nex3 Oct 10, 2019

Contributor

Long line


<==> README
This is designed to test the order of `@use` and `@forward` rules from multiple
sources.

This comment has been minimized.

Copy link
@nex3

nex3 Oct 10, 2019

Contributor

It's probably a good idea to describe the ordering constraints involved here.

@jathak jathak merged commit 2f1e09b into master Oct 10, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@jathak jathak deleted the use-order branch Oct 10, 2019
@nex3

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

Forgot to mention in the code review, but this should also have a CHANGELOG entry.

@jathak

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

Added it in #113.

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.