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

Update spec for relative imports in compileString() #3278

Merged
merged 2 commits into from Apr 12, 2022
Merged

Update spec for relative imports in compileString() #3278

merged 2 commits into from Apr 12, 2022

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Apr 3, 2022

The current documentation for the url field in StringOptionsWithoutImporter is incorrect that it does not match the behavior defined in js-api-spec and actual implementation:

    it('url is used to resolve relative loads', () =>
      sandbox(dir => {
        dir.write({'foo/bar/_other.scss': 'a {b: c}'});

        expect(
          compileString('@use "other";', {
            url: dir.url('foo/bar/style.scss'),
          }).css
        ).toBe('a {\n  b: c;\n}');
      }));

@nex3 nex3 self-requested a review April 6, 2022 20:29
@nex3
Copy link
Contributor

nex3 commented Apr 6, 2022

This behavior doesn't match the behavior specified here. (Specifically, the part that says "If url is not null and importer is null, set importer to a function that always returns null.") However, after talking it over we've decided that the current actual behavior is likely to be more intuitive for users, so we should stick with it and update the documentation.

tl;dr: Please update the "Compiling a String" procedure to reflect this behavior as well.

spec/spec.md Outdated Show resolved Hide resolved
@ntkme ntkme requested a review from nex3 April 7, 2022 00:40
@nex3
Copy link
Contributor

nex3 commented Apr 7, 2022

I'm treating this as a fast-track proposal. Although the spec change itself is backwards-incompatible, the new specification wording matches the existing behavior of the reference implementation so this is not considered a backwards-incompatible change. I'll leave this open for two workdays from now in case anyone has any objections to this change.

nex3 added a commit to sass/sass-spec that referenced this pull request Apr 7, 2022
The newly-tested behavior matches the specification as updated by
sass/sass#3278.
@ntkme ntkme changed the title Fix incorrect documentation for url in StringOptionsWithoutImporter Update spec for relative imports in compileString() Apr 9, 2022
@ntkme
Copy link
Contributor Author

ntkme commented Apr 9, 2022

List of related PRs to make implementations consistent with the spec update:

@nex3 PTAL. Thanks!

@nex3 nex3 merged commit 12cee2d into sass:main Apr 12, 2022
nex3 added a commit to sass/sass-spec that referenced this pull request Apr 12, 2022
The newly-tested behavior matches the specification as updated by
sass/sass#3278.
@ntkme ntkme deleted the options-url branch April 12, 2022 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

url documentation does not match js-api-spec
2 participants