Fix relative imports.

[jamestalmage]

UPDATE: Only discussing the fixes to relative imports here. See #31 for the percent char issue fix as a separate PR.

The fix for includePaths included in v1.0.0 breaks all relative imports.

While investigating a proper fix, I also noticed that one of the existing tests was failing in node-sass@4.11 (but worked fine in node-sass@4.10). For some reason, placeholders immediately followed by a % char were causing syntax errors.

I fixed the % char problem in the first commit, and relative imports in the second. Both commit comments contain more detailed explanations.

The relative @import fix should definitely be incorporated.

I suspect the other might actually just be a node-sass bug (I don't know enough about the syntax to say for sure). That said, It's a 4 line fix that's easy enough to back out later.

[giuseppeg]

thanks for the PR! can you split it up in two? I like to keep them scoped

[giuseppeg]

we should keep this I guess, tests should still pass with the old setup

[jamestalmage]

No, that test explicitly uses a relative import (i.e. starts with ./). The expected behavior is that it will resolve relative to the current file.

[jamestalmage]

In other words. The test as is, is verifying an undesirable behavior. You shouldn't have to add to includePaths if you're explicitly using ./ to signal a relative path.

[giuseppeg]

I am not sure about this, I thought with v1 we agreed that this should work when setting the includePath via sassOptions. cc @atombender

[jamestalmage]

Relative paths do not work properly in v1, they can easily resolve to the wrong file if you have two files with the same name in different directories

# directory structure
components
  foo-component
    styles.scss
    foo-component.js    # @import './styles'
  bar-component
    styles.scss
    bar-component.js    # @import './styles' 

sassOptions.includePaths = [
  "components/foo-component",
  "components/bar-component"
]

In this scenario, with your current implementation, it is going to search in order of the includePaths. This means both imports will resolve to the file inside foo-component. That's definitely not what bar-component wanted.

I am happy to add a regression tests that proves all that ⬆️ if it increases your comfort level, but we're really just testing node-sass implementation details at that point. The test as I've got it checks that explicit relative imports will work without modifying includePaths.

The problem you resolved in v1 was that you were clobbering the users intended includePaths settings in an effort to get relative paths to work.

[giuseppeg]

Gotcha thank you for the exhaustive explanation. How do you solve this issue in a standalone node-sass setup?

[jamestalmage]

The problem with the current implementation is that node-sass doesn't know the file location, it is only being provided the files contents. In a standalone node-sass setup, that would never happen (it's reading in the file contents itself, and it already knows the file path from the CLI or config).

This is simply giving node-sass the extra bit of information it needs to process the @import command properly.

[jamestalmage]

Just so we're clear. The previous attempt to make relative imports work went about it by adding the files location to includePaths:

styled-jsx-plugin-sass/index.js

Lines 17 to 19 in f7e3d5a

	if (settings.babel && settings.babel.filename) {
	includePaths.push(path.dirname(settings.babel.filename));
	}

That wasn't a great implementation:

1. It pushed the path at the end of includePaths, so it was looking in the same directory last, instead of first.
2. It did not make a defensive copy of includePaths, but kept appending to the end, so the contents of includePaths was dependent on file traversal order, and it would keep growing over time on the server end.

This implementation does not suffer from either of those pitfalls.