Skip to content

Fix relative imports. #30

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 1 commit into
base: master
Choose a base branch
from
Open

Fix relative imports. #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ module.exports = (css, settings) => {
const optionData = settings.sassOptions && settings.sassOptions.data || "";
const data = optionData + "\n" + cssWithPlaceholders;

const file = settings.babel && settings.babel.filename;
Copy link
Owner

@giuseppeg giuseppeg Jan 5, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@jamestalmage jamestalmage Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@giuseppeg giuseppeg Jan 5, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@jamestalmage jamestalmage Jan 19, 2019

Choose a reason for hiding this comment

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

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.


const preprocessed = sass.renderSync(
Object.assign(
{},
{file},
settings.sassOptions,
{ data }
)).css.toString()
Expand Down
3 changes: 0 additions & 3 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,6 @@ describe('styled-jsx-plugin-sass', () => {

assert.equal(
plugin(file.toString(), {
sassOptions: {
Copy link
Owner

@giuseppeg giuseppeg Jan 5, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@jamestalmage jamestalmage Jan 5, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Author

@jamestalmage jamestalmage Jan 5, 2019

Choose a reason for hiding this comment

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

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.

includePaths: [path.join(__dirname, 'fixtures')]
},
babel: { filename }
}).trim(),
cleanup(`
Expand Down