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

(Still) Wrong path for font scss in video-js.scss on v8.0.4 #8149

Closed
BrainCrumbz opened this issue Feb 21, 2023 · 17 comments · Fixed by #8681
Closed

(Still) Wrong path for font scss in video-js.scss on v8.0.4 #8149

BrainCrumbz opened this issue Feb 21, 2023 · 17 comments · Fixed by #8681
Labels
bug needs: triage This issue needs to be reviewed

Comments

@BrainCrumbz
Copy link
Contributor

BrainCrumbz commented Feb 21, 2023

Description

Apparently we met the issue mentioned at #7863 .

Reduced test case

No response

Steps to reproduce

  1. Clone the git repository
  2. rum npm install
  3. run npm test. Also, one can run npm run build afterwards.

Expected result:
All should be fine.

Actual result:
scripts show errors.

Errors

[... omitted ...]
> video.js@8.0.4 build:css
> npm-run-all build:css:*


> video.js@8.0.4 build:css:cdn
> sass --load-path='./' --load-path='./node_modules/' --no-source-map src/css/vjs-cdn.scss dist/alt/video-js-cdn.css

Error: Can't find stylesheet to import.
  ╷
5 │ @import "videojs-font/scss/icons";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  src\css\video-js.scss 5:9  @import
  src\css\vjs-cdn.scss 3:9   root stylesheet
ERROR: "build:css:cdn" exited with 65.
ERROR: "build:css" exited with 1.

The errors seem the same for test or build.

What version of Video.js are you using?

8.0.4 up to commit a27ee05

Video.js plugins used.

None

What browser(s) including version(s) does this occur with?

None. This is from command line

What OS(es) and version(s) does this occur with?

Windows 11
npm 9.3.1
node v18.14.0

Both from windows command line as well as WSL terminal.

@BrainCrumbz BrainCrumbz added bug needs: triage This issue needs to be reviewed labels Feb 21, 2023
@BrainCrumbz
Copy link
Contributor Author

Is there any workaround (even temporary) for this one? So that we can run tests and give a go to #8136

@BrainCrumbz
Copy link
Contributor Author

Is it possibile that issue is caused by using a different version of npm? When we run npm install, it says that package-lock.json file was created with an old version of npm. Can it be?

@amtins
Copy link
Contributor

amtins commented Mar 14, 2023

@BrainCrumbz I just tested to see if we could possibly run into the same error. However, I did not encounter this issue. But maybe I didn't follow the procedure as described.

Here is what I did:

  • I've node v18.15.0, npm v9.5.0
  • I clone de repo git@github.com:videojs/video.js.git, with the latest version 8.2.0
  • I CD into the folder
  • I ran npm install
  • I ran npm test
  • I ran npm run build

Everything went smoothly, see the logs:

> video.js@8.2.0 build:css
> npm-run-all build:css:*


> video.js@8.2.0 build:css:cdn
> sass --load-path='./' --load-path='./node_modules/' --no-source-map src/css/vjs-cdn.scss dist/alt/video-js-cdn.css


> video.js@8.2.0 postbuild:css:cdn
> postcss --verbose --config postcss.config.js -d dist/alt dist/alt/video-js-cdn.css

Processing dist/alt/video-js-cdn.css...
Finished dist/alt/video-js-cdn.css in 260 ms

> video.js@8.2.0 build:css:default
> sass --load-path='./' --load-path='./node_modules/' --no-source-map src/css/vjs.scss dist/video-js.css


> video.js@8.2.0 postbuild:css:default
> postcss --verbose --config postcss.config.js -d dist/ dist/video-js.css

Processing dist/video-js.css...
Finished dist/video-js.css in 247 ms

@amtins
Copy link
Contributor

amtins commented Mar 14, 2023

Same result with videojs 8.0.4

> video.js@8.0.4 build:css
> npm-run-all build:css:*


> video.js@8.0.4 build:css:cdn
> sass --load-path='./' --load-path='./node_modules/' --no-source-map src/css/vjs-cdn.scss dist/alt/video-js-cdn.css


> video.js@8.0.4 postbuild:css:cdn
> postcss --verbose --config postcss.config.js -d dist/alt dist/alt/video-js-cdn.css

Processing dist/alt/video-js-cdn.css...
Finished dist/alt/video-js-cdn.css in 252 ms

> video.js@8.0.4 build:css:default
> sass --load-path='./' --load-path='./node_modules/' --no-source-map src/css/vjs.scss dist/video-js.css


> video.js@8.0.4 postbuild:css:default
> postcss --verbose --config postcss.config.js -d dist/ dist/video-js.css

Processing dist/video-js.css...
Finished dist/video-js.css in 254 ms

@BrainCrumbz
Copy link
Contributor Author

Thanks for your info. Is that a Windows machine?

@amtins
Copy link
Contributor

amtins commented Mar 14, 2023

@BrainCrumbz I'm using an Ubuntu 22.04 machine. I add the following warning, since the package-lock was generated with an "older" npm version. But it shouldn't have any side effect.

npm WARN old lockfile 
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile 
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile 

Did you try to delete the dist folder and the node_modules to see if it fixes the issue ?

@BrainCrumbz
Copy link
Contributor Author

Yes we see that warning on package-lock too. Just for completeness:

  • we tried with video.js v8.2.0 as well as most recent commit, 35c539d
  • we tried both on windows terminal as well as Windows Linux Subsystem with Ubuntu (no linux machine available at the moment)
  • we deleted both dist and node_modules and installed/built again

with no effect on the issue.

What are the contents of videojs-font\scss after npm i? Because we see only these 2 files:

videojs-icons.scss
_icons.scss

and I don't know if that's consistent with the code importing:

@import "videojs-font/scss/icons";

I mean, there's no file named icons but actually it's _icons, not sure if that's the way it is supposed to be

@BrainCrumbz
Copy link
Contributor Author

ok, just checked on Sass guide, the underscore is expected

@BrainCrumbz
Copy link
Contributor Author

As a (temporary?) workaround this is the change that enabled us to build and test successfully:

@import "../../node_modules/videojs-font/scss/icons";

@lihqi
Copy link

lihqi commented May 20, 2023

I had the same problem with my project using lerna to manage packages

@fauzi9331
Copy link

fauzi9331 commented Jul 18, 2023

I had the same problem and managed to fix it by removing ' in --load-path in package.json

before:

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

after

"build:css:cdn": "sass --load-path=./ --load-path=./node_modules/ --no-source-map src/css/vjs-cdn.scss dist/alt/video-js-cdn.css",

But i'm not sure about its side effect

Windows 11
npm 9.6.7
node v20.3.1

@ahm23
Copy link

ahm23 commented Oct 19, 2023

@fauzi9331 's solution works for me as well. Remove the quotes around the "load paths" for both "build:css:cdn" and "build:css:default" in package.json and everything builds with no issues.

Windows 10
Node v18.18.0
npm v10.1.0

@BrainCrumbz
Copy link
Contributor Author

We just forked main today, so video.js is 8.11.8. Following @ahm23 integrations worked for us, that is: we had to modify both build:css:cdn and build:css:default tasks in package.json. Only modifying the first task was not enough for us.

Windows 11, 23H2
node 18.14.0
npm 9.3.1

@mister-ben
Copy link
Contributor

Do escaped double quotes work on people's environments that have problems with single quotes?, e.g. "sass --load-path=\"./\" …

@BrainCrumbz
Copy link
Contributor Author

Hi there

just tried with these changes, the way you suggested:

immagine

npm run build completed with no errors. Same for npm run start. Would that also work on Unix/MacOS machines?

@mister-ben
Copy link
Contributor

I think that should work everywhere, yes

@BrainCrumbz
Copy link
Contributor Author

Good news. If you need some more tests on Windows, we could try. Also WSL with Ubuntu.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs: triage This issue needs to be reviewed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants