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

Improve importing files through load paths and node_modules #102

Merged
merged 9 commits into from Oct 1, 2019

Conversation

@nex3
Copy link
Contributor

commented Oct 1, 2019

Most pertinently, we no longer migrate files in those locations, and
we properly generate @uses for dependencies within them.

nex3 added 6 commits Oct 1, 2019
This error was already thrown when different results were generated
due to different entrypoints, but not for the same entrypoint. In
practice I don't think this can come up with the module migrator,
though, due to #99.
We had been relying on the _lastUrl instance variable, but this is
cleaner and works even if we don't actually visit the stylesheet in
question.
Now that we have References, we don't need the MigrationVisitor to
visit files we aren't actually going to update.
Importer.canonicalize() should always be able to handle any URLs it
itself returns, or URLs relative to those URLs.
Rather than just using the original import string, which may be a
relative URL, we now get the importer itself to decanonicalize the URL
into whichever input form it usually has.

Closes #100
@nex3 nex3 requested a review from jathak Oct 1, 2019
This ensures that we don't end up migrating libraries that users don't
have control over.

Closes #98
@nex3 nex3 force-pushed the stop-at-load-path branch from 1df1f9a to 7381703 Oct 1, 2019
// visit it again and throw an error if it produces different results
// (#99).
var url = result.item2.span.sourceUrl;
if (_importers.containsKey(url)) return;

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

We need to revisit duplicate imports in _ReferenceVisitor (but not necessarily in _ModuleMigrationVisitor) to avoid breaking cases like this:

<==> entrypoint.scss
@import "a";
@import "b";

a {
  color: fn();
}

<==> _a.scss
@import "b";
@function fn() {@return blue;}

<==> _b.scss
@function fn() {@return green;}

Pre-migration, fn() within entrypoint should refer to the one in b, but if we only visit b the first time it's encountered, the definition in a will override it by the time the reference is visited and the main migrator will then use the wrong namespace.

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

A good example of why it's important to have thorough test cases 😄! Done.

@@ -361,10 +368,15 @@ class _ReferenceVisitor extends RecursiveAstVisitor {

/// Given a URL from a `@use` or `@forward` rule, loads and visits the
/// stylesheet it points to and returns its canonical URL.
Uri _loadUseOrForward(Uri ruleUrl) {
Uri _loadUseOrForward(Uri ruleUrl, AstNode nodeForSpan) {

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

Is there any reason that this needs to be AstNode and not SassNode?

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

Just consistency with the convention from the Sass codebase.

/// Immediately throw a [MigrationException] when a missing dependency is
/// encountered, as the module migrator needs to traverse all dependencies.
@override
void handleMissingDependency(Uri dependency, FileSpan context) {

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

Since this method only existed because the _ModuleMigrationVisitor needed to override it, it probably make sense to eliminate it from MigrationVisitor.

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

Done.

@@ -74,8 +75,11 @@ abstract class Migrator extends Command<Map<Uri, String>> {
Map<Uri, String> run() {
var allMigrated = <Uri, String>{};
var importer = FilesystemImporter('.');

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

Is there a reason why this is not also a ReversibleFilesystemImporter?

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

URLs that are relative to the entrypoint should always be converted to relative URLs. In fact, generating a @use whose URL is relative to the working directory would be incorrect, since the working directory is not on the load path.

// If [dependency] comes from a non-relative import, don't migrate it,
// because it's likely to be outside the user's repository and may even be
// authored by a different person.
if (result.item1 != _importer) return;

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

This definitely makes sense for the default behavior, but would it make sense to add a flag to override this?

I think there are some stylesheets within Google that specify all dependencies relative to a load path.

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

Added a TODO.

@nex3 nex3 requested a review from jathak Oct 1, 2019
@jathak
jathak approved these changes Oct 1, 2019
/// Migrators should override this if they want a different behavior.
@protected
void handleMissingDependency(Uri dependency, FileSpan context) {
_missingDependencies.putIfAbsent(

This comment has been minimized.

Copy link
@jathak

jathak Oct 1, 2019

Member

Tests are failing because this needs to be inlined into visitDependency

This comment has been minimized.

Copy link
@nex3

nex3 Oct 1, 2019

Author Contributor

Done.

@nex3 nex3 merged commit cb23adc into master Oct 1, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@nex3 nex3 deleted the stop-at-load-path branch Oct 1, 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.