Add support for running in the browser #1895

jerivas · 2023-02-24T21:41:19Z

WIP, but this gets us closer to fixing #25

Fix self/window issues (Enable including libraries with a preamble in the browser directly mbullington/node_preamble.dart#29)
Generate ESM bundles (Generate ESM imports for JS on the browser google/dart_cli_pkg#118)
More resilient access to process.*
Throw if trying to access the file system
Let users compile stylesheets without importers, but improve error message if trying to import a stylesheet without explicit importers
Throw errors if trying to use compile(), compileAsync(), render(), and renderSync() in the browser
Make sure logging still works in browser contexts
Add basic tests that run in the browser
Run JS API tests in the browser. Add browser tests for the JS API in the CI for both this and the sass/sass-spec repo. See Add support for running JS Spec Tests in browser context. sass-spec#1902

lib/src/io/node.dart

pubspec.yaml

nex3 · 2023-02-24T22:22:33Z

I may have mispoken on the call earlier—compile{,ToResult}{,Async} are all fundamentally not going to work on the web. They take a filesystem path as a parameter, not a URL, and there's no real way to make that make sense in a web context. They should all just throw errors indicating that they're not supported and users should use compileStringToResult{,Async} instead.

tool/grind.dart

lib/src/io/node.dart

jerivas

I added some basic browsers tests. Most of them are already passing, but I'm having trouble witha few things

.github/workflows/ci.yml

test/browser_test.dart

* main: Require node>=14.0.0 (sass#1911) Add support for constants in calculations (sass#1922)

* main: Remove Node 12 from CI (sass#1925)

pubspec.yaml

jgerigmeyer

@nex3 @jerivas I think this is ready for another (close to final?) round of review. With the changes in 4cb6b77 to enable logging, and the changes in https://github.com/sass/dart-sass/pull/1895/files/4cb6b772f55c82ca083dffcd786ead16580bf6dd..ae772f5f66fb5096ce299485375cf79f4e0f189d to run the JS API spec tests as part of CI, I'm not aware of any further items on the to-do list?

jgerigmeyer · 2023-05-03T15:12:08Z

.github/workflows/ci.yml

+
+      - name: Check out sass-spec
+        uses: sass/clone-linked-repo@v1
+        # TODO update this to use "sass/sass-spec" directly


Remember to update this before merging.

If you send a PR to the official sass-spec repo, you can link to it in the PR description here and this PR will automatically run against that one.

jgerigmeyer · 2023-05-03T16:01:15Z

lib/src/io/node.dart

+    if (_stderr == null) {
+      console.error("${object ?? ''}\n");
+    } else {
+      _stderr?.write("${object ?? ''}\n");
+    }


This works (as in, it does log), but I think it's probably not ideal?

I think a better solution than stubbing out stderr in the browser might be to have a separate BrowserLogger that directly calls console.error().

@nex3 What do you think of the revised approach in d30dd42?

nex3

Other than the logger stuff, this is looking very good!

nex3 · 2023-05-08T23:21:41Z

.github/workflows/ci.yml

+
+      - name: Check out sass-spec
+        uses: sass/clone-linked-repo@v1
+        # TODO update this to use "sass/sass-spec" directly


If you send a PR to the official sass-spec repo, you can link to it in the PR description here and this PR will automatically run against that one.

nex3 · 2023-05-08T23:33:24Z

lib/src/io/node.dart

+    if (_stderr == null) {
+      console.error("${object ?? ''}\n");
+    } else {
+      _stderr?.write("${object ?? ''}\n");
+    }


I think a better solution than stubbing out stderr in the browser might be to have a separate BrowserLogger that directly calls console.error().

nex3 · 2023-05-08T23:34:04Z

lib/src/io/node.dart

-  void write(Object object) => _stderr.write(object.toString());
+  void write(Object object) {
+    if (_stderr == null) {
+      console.error(object.toString());


Note that stderr.write() doesn't append a newline, while console.error() does. If we do decide we need to fake stderr in the browser, we'll need to build up a buffer here and only emit it once we hit an actual newline.

* main: (162 commits) Move source and test files to namespaced subdirectories Update Dart Sass version and release Update Dart Sass version and release Update Dart Sass version and release Update Dart Sass version and release Update Dart Sass version and release Update Dart Sass version and release Revert "Remove workaround for dart-lang/setup-dart#59 (sass#151)" (sass#153) Update Dart Sass version and release Update Dart Sass version and release Remove workaround for dart-lang/setup-dart#59 (sass#151) Update Dart Sass version and release Update Dart Sass version and release Fix qemu releases (sass#149) Update Dart Sass version and release Bump sass_api from 4.2.1 to 5.0.0 (sass#143) Bump meta from 1.8.0 to 1.9.0 (sass#144) Bump grinder from 0.9.2 to 0.9.3 (sass#145) Add missing setup-dart step in qemu release (sass#147) Use buf instead of protoc to compile protobufs (sass#146) ...

nex3

A few relatively small comments, but overall looks great!

nex3 · 2023-05-16T20:54:13Z

lib/src/io/interface.dart

+abstract class Console {
+  external void log(String data);
+  external void info(String data);
+  external void warn(String data);
+  external void error(String data);
+}
+
+Console? get console => throw '';


The IO library is really for interfaces that are shared across the VM and JS. Something like the console that's only ever available in JS should go in a separate library in lib/src/node/.

nex3 · 2023-05-16T20:56:05Z

lib/src/io/node.dart

 import 'package:node_interop/fs.dart';
-import 'package:node_interop/node_interop.dart';
+import 'package:node_interop/node_interop.dart' as n hide process;


You should only need to either import this with a prefix or hide a conflicting name, not both.

nex3 · 2023-05-16T21:03:57Z

lib/src/logger.dart

@@ -21,7 +23,12 @@ abstract class Logger {

  /// Creates a logger that prints warnings to standard error, with terminal
  /// colors if [color] is `true` (default `false`).
-  const factory Logger.stderr({bool color}) = StderrLogger;
+  factory Logger.stderr({bool color = false}) {


Unfortunately, converting a factory constructor from const to non-const is a breaking change in Dart. This means that instead of a separate BrowserLogger class, you'll need to use if (isBrowser) in StderrLogger itself. Sorry I didn't catch this earlier.

nex3 · 2023-05-16T22:36:07Z

.github/workflows/ci.yml

@@ -218,6 +218,54 @@ jobs:
        run: npm run js-api-spec -- --sassPackage ../embedded-host-node --sassSassRepo ../language
        working-directory: sass-spec

+  sass_spec_js_browser:
+    name: "JS API Tests | Pure JS | Dart ${{ matrix.dart_channel }} | Browser"


To make this a little easier to differentiate in the web UI, let's make it

Suggested change

name: "JS API Tests | Pure JS | Dart ${{ matrix.dart_channel }} | Browser"

name: "JS API Tests | Browser | Dart ${{ matrix.dart_channel }}"

since "browser" implies "pure JS" anyway.

* main: Use the embedded protocol from the Sass language repo (sass#1966) Format with dart 3.0.1 (sass#1967) Pull more repeated GH Action tasks into sub-actions (sass#1964) Fix printing usage when no args provided (sass#1963)

jgerigmeyer

@nex3 Ready for another round of review (564a4ce).

jgerigmeyer · 2023-05-18T15:19:47Z

.github/util/initialize/action.yml

@@ -10,7 +10,7 @@ runs:
  steps:
    - uses: dart-lang/setup-dart@v1
      with:
-        sdk: "${{ inputs.sdk }}"
+        sdk: "${{ inputs.dart-sdk }}"


@nex3 I think this is a typo in the main branch.

jgerigmeyer · 2023-05-18T15:21:51Z

lib/src/logger/stderr.dart

@@ -9,8 +9,9 @@ import 'package:stack_trace/stack_trace.dart';
 import '../io.dart';
 import '../logger.dart';
 import '../utils.dart';
+import '../node/console_stub.dart' if (dart.library.js) '../node/console.dart';


@nex3 Is there a better way to do this that still avoids Dart library 'dart:js_util' is not available on this platform errors?

Unfortunately no :(. The Dart VM still needs to know how to typecheck the APIs even if it won't actually run them in practice.

I think, in retrospect, that the cleanest approach here is probably to hide even more of the implementation and just expose a printError() function. I don't want to make you undo all the refactors so I've just gone and made the change myself.

@nex3 Makes sense, and LGTM! 🚀

jgerigmeyer · 2023-05-18T15:22:28Z

lib/src/node/console_stub.dart

+// Copyright 2023 Google Inc. Use of this source code is governed by an
+// MIT-style license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/MIT.
+
+abstract class Console {
+  external void log(String data);
+  external void info(String data);
+  external void warn(String data);
+  external void error(String data);
+}
+
+Console get console => throw '';


This seems a bit silly, but it doesn't seem like you can conditionally import a package without also having a fallback package.

nex3 · 2023-05-19T00:11:52Z

lib/src/logger/stderr.dart

@@ -9,8 +9,9 @@ import 'package:stack_trace/stack_trace.dart';
 import '../io.dart';
 import '../logger.dart';
 import '../utils.dart';
+import '../node/console_stub.dart' if (dart.library.js) '../node/console.dart';


Unfortunately no :(. The Dart VM still needs to know how to typecheck the APIs even if it won't actually run them in practice.

I think, in retrospect, that the cleanest approach here is probably to hide even more of the implementation and just expose a printError() function. I don't want to make you undo all the refactors so I've just gone and made the change myself.

jerivas added 2 commits February 24, 2023 21:32

Install node_preamble from git

cc4dfdc

Handle missing process

754f30c

jerivas commented Feb 24, 2023

View reviewed changes

lib/src/io/node.dart Outdated Show resolved Hide resolved

pubspec.yaml Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

jerivas mentioned this pull request Mar 3, 2023

Provide a useful default importer for the browser oddbird/sass-browser#1

Closed

Upgrade dependencies

42122ed

jerivas commented Mar 6, 2023

View reviewed changes

tool/grind.dart Outdated Show resolved Hide resolved

jerivas added 5 commits March 6, 2023 23:55

Address review

f0a1604

Fix tests

e0b8773

Correctly type process

0a8b830

Restore workaround for pulyaevskiy/node-interop#93

a713c13

Go back to stable cli_pkg

f08d8ca

nex3 requested changes Mar 8, 2023

View reviewed changes

lib/src/io/node.dart Outdated Show resolved Hide resolved

lib/src/io/node.dart Outdated Show resolved Hide resolved

lib/src/io/node.dart Show resolved Hide resolved

lib/src/io/node.dart Outdated Show resolved Hide resolved

lib/src/io/node.dart Outdated Show resolved Hide resolved

jerivas and others added 2 commits March 14, 2023 22:33

Guard function invocations when process is missing

2dcf8cd

Merge branch 'main' into browser

8dfcb28

This comment was marked as outdated.

Sign in to view

Add basic browser tests

ae325dd

jerivas commented Mar 17, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

test/browser_test.dart Outdated Show resolved Hide resolved

magic-akari mentioned this pull request Mar 20, 2023

Fix the execution of binary script cannot load the library with jsEsmExports google/dart_cli_pkg#123

Merged

jgerigmeyer and others added 2 commits March 23, 2023 14:33

upgrade cli_pkg

e17dc57

Rework tests to use @js()

7d36bf7

jerivas commented Mar 24, 2023

View reviewed changes

test/browser_test.dart Outdated Show resolved Hide resolved

jgerigmeyer added 6 commits March 24, 2023 13:38

Merge branch 'main' into browser

81e36d4

* main: Require node>=14.0.0 (sass#1911) Add support for constants in calculations (sass#1922)

Only run browser tests when using Chrome platform

e238290

remove unused tag definition

d27084c

Merge branch 'main' into browser

46f4cc9

* main: Remove Node 12 from CI (sass#1925)

Move @JS("URL") to constructor instead of entire class

b669b35

Try adding @anonymous?

7e51c9b

jgerigmeyer reviewed Mar 29, 2023

View reviewed changes

pubspec.yaml Outdated Show resolved Hide resolved

jgerigmeyer added 4 commits May 2, 2023 11:32

Temp use sass-spec PR

99763e0

Another attempt to use sass-spec PR

a093f24

empty to trigger CI

2f4cb37

Install built JS deps before running JS API spec tests

ae772f5

jgerigmeyer reviewed May 3, 2023

View reviewed changes

nex3 reviewed May 8, 2023

View reviewed changes

jgerigmeyer added 4 commits May 12, 2023 16:51

address review

a7ec7d1

fixes for dart v3

2a15647

typo

ec59edd

Revised attempt at browser logging

d30dd42

jerivas requested a review from nex3 May 16, 2023 15:51

jgerigmeyer added 3 commits May 16, 2023 12:43

Add missing CI call

f598cee

Mark new tests for vm

c2fe97b

nex3 approved these changes May 16, 2023

View reviewed changes

nex3 reviewed May 16, 2023

View reviewed changes

jgerigmeyer added 3 commits May 18, 2023 09:52

Merge branch 'main' into browser

e4fa5aa

* main: Use the embedded protocol from the Sass language repo (sass#1966) Format with dart 3.0.1 (sass#1967) Pull more repeated GH Action tasks into sub-actions (sass#1964) Fix printing usage when no args provided (sass#1963)

update ci

0d0d136

Another attempt at browser logging

564a4ce

jgerigmeyer reviewed May 18, 2023

View reviewed changes

Style changes, move to a printError() function

f60225e

nex3 approved these changes May 19, 2023

View reviewed changes

nex3 added 3 commits May 18, 2023 17:34

Use printStderr in bin/sass.dart

45f353d

Match old newlines

2e5c54f

Synchronize

8dbf5b3

ntkme mentioned this pull request May 19, 2023

'fs Module not found #1969

Closed

nex3 merged commit cca9464 into sass:main May 19, 2023
45 checks passed

jgerigmeyer deleted the browser branch May 19, 2023 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for running in the browser #1895

Add support for running in the browser #1895

jerivas commented Feb 24, 2023 •

edited

This comment was marked as outdated.

nex3 commented Feb 24, 2023

This comment was marked as outdated.

jerivas left a comment

jgerigmeyer left a comment

jgerigmeyer May 3, 2023

nex3 May 8, 2023

jgerigmeyer May 12, 2023

jgerigmeyer May 3, 2023

nex3 May 8, 2023

jgerigmeyer May 15, 2023

nex3 left a comment

nex3 May 8, 2023

nex3 May 8, 2023

nex3 May 8, 2023

nex3 left a comment

nex3 May 16, 2023

nex3 May 16, 2023

nex3 May 16, 2023

nex3 May 16, 2023

jgerigmeyer left a comment

jgerigmeyer May 18, 2023

jgerigmeyer May 18, 2023

nex3 May 19, 2023 •

edited

jgerigmeyer May 19, 2023

jgerigmeyer May 18, 2023

nex3 May 19, 2023 •

edited

	name: "JS API Tests \| Pure JS \| Dart ${{ matrix.dart_channel }} \| Browser"
	name: "JS API Tests \| Browser \| Dart ${{ matrix.dart_channel }}"

Add support for running in the browser #1895

Add support for running in the browser #1895

Conversation

jerivas commented Feb 24, 2023 • edited

This comment was marked as outdated.

nex3 commented Feb 24, 2023

This comment was marked as outdated.

jerivas left a comment

Choose a reason for hiding this comment

jgerigmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgerigmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 May 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 May 19, 2023 • edited

Choose a reason for hiding this comment

jerivas commented Feb 24, 2023 •

edited

nex3 May 19, 2023 •

edited

nex3 May 19, 2023 •

edited