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

Use the index of stdin instead of zero #681

Merged
merged 4 commits into from
May 7, 2019
Merged

Use the index of stdin instead of zero #681

merged 4 commits into from
May 7, 2019

Conversation

FractalBoy
Copy link
Contributor

@FractalBoy FractalBoy commented May 7, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Solves issue #671, which breaks the use of resolve-url-loader

Breaking Changes

None

Additional Info

This will look for "stdin" in the list of sources and replace it with the Sass file. If it can't find the string for whatever reason, it will fall back to the previous functionality and just use the 0th index not do anything.

fixes #671

@jsf-clabot
Copy link

jsf-clabot commented May 7, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #681 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
+ Coverage   98.02%   98.06%   +0.03%     
==========================================
  Files           6        6              
  Lines         152      155       +3     
==========================================
+ Hits          149      152       +3     
  Misses          3        3
Impacted Files Coverage Δ
lib/loader.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9162e45...34f14b8. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, can you add tests?

@FractalBoy
Copy link
Contributor Author

I think the test cases actually do cover this, but I can't figure out how to make them break.

stdin is in the 0th position of result.map.sources for both dart-sass and node-sass.

@alexander-akait
Copy link
Member

@FractalBoy hm, it is very strange, for sass(dart-sass) stdin should be at end, maybe need update sass (dart-sass) to latest version, anyway good job

@FractalBoy
Copy link
Contributor Author

Upgrading sass to 1.20.1 didn't break any tests in master.

lib/loader.js Outdated
result.map.sources[stdinIndex] = path.relative(
process.cwd(),
resourcePath
);
Copy link
Member

@alexander-akait alexander-akait May 7, 2019

Choose a reason for hiding this comment

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

Ok, thanks for investigation, maybe rewrite this like:

if (stdinIndex !== -1) {
  result.map.sources[stdinIndex] = path.relative(
        process.cwd(),
        resourcePath
      );
}

This allow to us protect code if stdin was excluded from sources in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand the point - the way I've written it, stdinIndex will never be -1. If it is, it will be 0.

Copy link
Member

Choose a reason for hiding this comment

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

@FractalBoy I am afraid what in future node-sass/sass can remove stdin from result.map.sources so better check stdin exists in sources and then modify

Copy link
Member

@alexander-akait alexander-akait May 7, 2019

Choose a reason for hiding this comment

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

Now - If stdin not exists in sources we break source map, because on 0 index will be contain path to source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would actually be better to remove stdin if it exists and then always add the path?

      const stdinIndex = result.map.sources.findIndex(
        (source) => source.indexOf('stdin') !== -1
      );

      const relativeResourcePath = path.relative(
        process.cwd(),
        resourcePath
      );

      if (stdinIndex !== -1) {
        result.map.sources[stdinIndex] = relativeResourcePath;
      } else {
        result.map.sources.push(relativeResourcePath);
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, nevermind, that would probably screw up the source maps since it wouldn't line up correctly.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job, thanks

@alexander-akait alexander-akait merged commit e279f2a into webpack-contrib:master May 7, 2019
@FractalBoy
Copy link
Contributor Author

@evilebottnawi do you know when your next release will be?

@alexander-akait
Copy link
Member

alexander-akait commented May 8, 2019

@FractalBoy this or next week my bad

@FractalBoy
Copy link
Contributor Author

@evilebottnawi awesome, thanks

@FractalBoy
Copy link
Contributor Author

@evilebottnawi will you be publishing a new release soon? i need this update for a project i'm working on.

@FractalBoy
Copy link
Contributor Author

@evilebottnawi when will your next release be? it's been over a month since you said it would be released.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 25, 2019

@FractalBoy sorry for delay, next week, there are a lot of work, sass-loader already on top of my todo

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.

Sourcemap entry path is 'stdin' with dart-sass
3 participants