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

Import failure with sass.compile(), sassEmbedded.renderSync() and sassEmbedded.compile() methods, but not with sass.renderSync() method #1920

Closed
Rezyan opened this issue Mar 21, 2023 · 4 comments · Fixed by sass/embedded-host-node#212
Assignees

Comments

@Rezyan
Copy link

Rezyan commented Mar 21, 2023

Introduction

Hi,

I create this issue on @nex3 request.

The issue is simple to understand, there is an import failure with sass.compile(), sassEmbedded.renderSync() and sassEmbedded.compile() methods, but not with sass.renderSync() method:

node:internal/process/esm_loader:97
    internalBinding('errors').triggerUncaughtException( 
                              ^

sass.Exception [Error]: Can't find stylesheet to import.
  ╷
5 │ @import "node_modules/bootstrap/scss/functions";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  node_modules\select2-bootstrap-5-theme\src\select2-bootstrap-5-theme.scss 5:9  @import
  style.scss 2:9                                                                 root stylesheet
    at Object.wrapException (C:\sass_import_failure\node_modules\sass\sass.dart.js:1237:17)
    at _EvaluateVisitor1._evaluate0$_loadStylesheet$4$baseUrl$forImport (C:\sass_import_failure\node_modules\sass\sass.dart.js:82636:19)
    at _EvaluateVisitor1._evaluate0$_loadStylesheet$3$forImport (C:\sass_import_failure\node_modules\sass\sass.dart.js:82665:19)
    at _EvaluateVisitor__visitDynamicImport_closure1.call$0 (C:\sass_import_failure\node_modules\sass\sass.dart.js:84417:21)
    at _EvaluateVisitor1._evaluate0$_withStackFrame$1$3 (C:\sass_import_failure\node_modules\sass\sass.dart.js:83672:25)
    at _EvaluateVisitor1._evaluate0$_withStackFrame$3 (C:\sass_import_failure\node_modules\sass\sass.dart.js:83678:19)
    at _EvaluateVisitor1._evaluate0$_visitDynamicImport$1 (C:\sass_import_failure\node_modules\sass\sass.dart.js:82598:19)
    at _EvaluateVisitor1.visitImportRule$1 (C:\sass_import_failure\node_modules\sass\sass.dart.js:82572:17)
    at ImportRule0.accept$1$1 (C:\sass_import_failure\node_modules\sass\sass.dart.js:87634:22)
    at ImportRule0.accept$1 (C:\sass_import_failure\node_modules\sass\sass.dart.js:87637:19)

Node.js v18.14.2

Summary table:

Sass package and version Method Expected behavior (no import failure)
[email protected] renderSync() ✔️
[email protected] compile()
[email protected] renderSync()
[email protected] compile()

Reproducible scenario

package.json:

{
	"type": "module",
	"dependencies": {
		"sass": "^1.59.3",
		"sass-embedded": "^1.59.3",
		"select2": "^4.1.0-rc.0",
		"select2-bootstrap-5-theme": "^1.3.0"
	}
}

style.scss:

@import './node_modules/select2/src/scss/core';
@import './node_modules/select2-bootstrap-5-theme/src/select2-bootstrap-5-theme';

index.js:

import sass from 'sass';
import sassEmbedded from 'sass-embedded';

const src = './style.scss';

sass.renderSync({file: src}); // ✔️ OK
sass.compile(src); // ❌ ISSUE
sassEmbedded.renderSync({file: src}); // ❌ ISSUE
sassEmbedded.compile(src); // ❌ ISSUE

Context

Related comment:

Node.js version: 18.14.2

@ntkme
Copy link
Contributor

ntkme commented Mar 21, 2023

I think the failure may be the expected behavior here at least in new API. In you example it is node_modules/select2-bootstrap-5-theme/src/select2-bootstrap-5-theme.scss trying to load node_modules/bootstrap/scss/functions.

  1. The import should not be resolved relatively, as node_modules/select2-bootstrap-5-theme/src/node_modules/bootstrap/scss/_functions.scss need to exists. Obviously, this won't exist simply due to where npm installs packages, that we can rule out this case.
  2. The current working directory is NOT in loadPath by default in new API, so that it is expected to fail in new API.
  3. The current working directory is NOT in loadPath by default in the legacy API. However, it turns out there is a special logic in NodeImporter to always allow relative resolve from the working directory. It was not good idea as it introduces inconsistency of outcome depends on where the sass command is running. sass-embedded's legacy API emulation does not have this logic and that's probably where the difference comes from:

/// Tries to load a stylesheet at the given [path] from a load path (including
/// the working directory).
///
/// Returns the stylesheet at that path and the URL used to load it, or `null`
/// if loading failed.
Tuple2<String, String>? _resolveLoadPath(String path, bool forImport) {
// 2: Filesystem imports relative to the working directory.
var cwdResult = _tryPath(p.absolute(path), forImport);
if (cwdResult != null) return cwdResult;

We can change the behavior of legacy API in sass-embedded to align with sass, but we will likely not change the behavior in new API.

@nex3 Any thoughts?

@Rezyan
Copy link
Author

Rezyan commented Mar 21, 2023

Anyway, whatever happens, I found a workaround. I think that the select2-bootstrap-5-theme library file contains relative path quirks. So please don't change any behavior just for me, I created the issue only because @nex3 asked to me.

@ntkme
Copy link
Contributor

ntkme commented Mar 21, 2023

You can explicitly add . (meaning current working directory) to loadPaths option in new API and it should work.

@nex3
Copy link
Contributor

nex3 commented Mar 27, 2023

Thanks for rooting this out, @Elysiome and @ntkme. In this case, we can't accurately emulate this behavior in the embedded host because the embedded host builds its legacy API support on top of the new API and the new API (by design) doesn't make it possible to distinguish between absolute and relative imports. I'm afraid this is one of the few cases where there's an unavoidable behavior difference between the two.

@nex3 nex3 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 27, 2023
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 a pull request may close this issue.

3 participants