fix (sass): import path has cwd once again

[tsi]

This fixes a sass compile error we got after upgrading to 7.x
Same as #4061 but for v7.x

[welcome]

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

• Run npm run lint -- --errors locally to catch formatting errors earlier.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[tsi]

I see now that this was done intentionally as part of a move to use dart-sass, correct?
3e10571

Do we have some kind of upgrade instruction for people coming from 5.x or 6.x ?
I just went over everything in webpack-contrib/sass-loader#435 and I still get the same error I did with node-sass

[gkatsev]

Yeah, this was done as part of the switch to dart-sass. As you can see the build is failing with this change. I'm not sure how to reconcile the two.
Maybe we can update the path after we build locally and before we publish so the published version has import 'node_modules/ but during build it has ../../node_modules?

[tsi]

I still don't get something. even if I move to our project to dart-sass, I still get the same error:

SassError: Can't find stylesheet to import.
      ╷
    5 │ @import "../../node_modules/videojs-font/scss/icons";
      │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And if I change to :
@import "node_modules/videojs-font/scss/icons";
Everything works like it should.
So I'm really confused. does this work anywhere else? what are we doing differently?

[gkatsev]

Oh, I wonder if this is happening because the location of files is different depending on whether it's used during the build in the video.js source directory vs being imported in sass from node_modules/video.js/src/css...
Also, potentially related to where videojs-font is installed along with video.js.

@tsi can you show me the output of npm ls video.js and npm ls videojs-font? Also, I assume just node_modules/... without the ../../ does work when you import it in your project?

[tsi]

location of files is different depending on whether it's used during the build in the video.js source directory

I think that's spot-on
when you develop and build video.js, the videojs-font package is located in video.js's (your repo's) /node_modules. but when I'm developing, the videojs-font package is located in my own /node_modules, right next to video.js.

can you show me the output of npm ls video.js and npm ls videojs-font

% npm ls video.js    
...
├─┬ [email protected] 
│ └─┬ @videojs/[email protected]
│   └── [email protected] 
├─┬ [email protected]
│ └── [email protected] 
├─┬ [email protected]
│ └── [email protected] 
├─┬ [email protected]
│ └── [email protected] 
└─┬ [email protected]
  └── [email protected] 

% npm ls videojs-font
...
├─┬ [email protected]
│ ├─┬ @videojs/[email protected]
│ │ └─┬ [email protected]
│ │   └── [email protected]  deduped
│ └── [email protected] 
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected] 
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected]  deduped
├─┬ [email protected]
│ └─┬ [email protected]
│   └── [email protected]  deduped
└─┬ [email protected]
  └─┬ [email protected]
    └── [email protected]  deduped

Also, I assume just node_modules/... without the ../../ does work

It does, and I assumed it should work for you too.

[gkatsev]

So, I think I found a workaround that won't require doing weird stuff. The sass api includes a --load-path option and if we use we can get it to build locally with the import being node_modules/....

@tsi Can you add --load-path='./' to the package.json lines for build:css:cdn and build:css:default? They should look like:

sass --load-path='./' --no-source-map src/css/vjs-cdn.scss dist/alt/video-js-cdn.css

and

sass --load-path='./' --no-source-map src/css/vjs.scss dist/video-js.css

[tsi]

Sure, done, will it pass the build test?

[gkatsev]

Looks like it passed, 🎉. We'll likely be able to release next week.

[tsi]

You're quick! Thanks!!

[tsi]

@gkatsev no merge?

[gkatsev]

It'll happen this week. We have a policy of requiring two approvals for PRs and the other approvers were out last week.

[misteroneill]

Thanks @tsi!

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉