-
Notifications
You must be signed in to change notification settings - Fork 351
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
Add support for running in the browser #1895
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
I may have mispoken on the call earlier— |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some basic browsers tests. Most of them are already passing, but I'm having trouble witha few things
* main: Remove Node 12 from CI (sass#1925)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
.github/workflows/ci.yml
Outdated
|
||
- name: Check out sass-spec | ||
uses: sass/clone-linked-repo@v1 | ||
# TODO update this to use "sass/sass-spec" directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to update this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
lib/src/io/node.dart
Outdated
if (_stderr == null) { | ||
console.error("${object ?? ''}\n"); | ||
} else { | ||
_stderr?.write("${object ?? ''}\n"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better solution than stubbing out stderr in the browser might be to have a separate BrowserLogger
that directly calls console.error()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the logger stuff, this is looking very good!
.github/workflows/ci.yml
Outdated
|
||
- name: Check out sass-spec | ||
uses: sass/clone-linked-repo@v1 | ||
# TODO update this to use "sass/sass-spec" directly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/src/io/node.dart
Outdated
if (_stderr == null) { | ||
console.error("${object ?? ''}\n"); | ||
} else { | ||
_stderr?.write("${object ?? ''}\n"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a better solution than stubbing out stderr in the browser might be to have a separate BrowserLogger
that directly calls console.error()
.
lib/src/io/node.dart
Outdated
void write(Object object) => _stderr.write(object.toString()); | ||
void write(Object object) { | ||
if (_stderr == null) { | ||
console.error(object.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few relatively small comments, but overall looks great!
lib/src/io/interface.dart
Outdated
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 ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
.
lib/src/io/node.dart
Outdated
import 'package:node_interop/fs.dart'; | ||
import 'package:node_interop/node_interop.dart'; | ||
import 'package:node_interop/node_interop.dart' as n hide process; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should only need to either import this with a prefix or hide a conflicting name, not both.
lib/src/logger.dart
Outdated
@@ -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}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.github/workflows/ci.yml
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a little easier to differentiate in the web UI, let's make it
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -10,7 +10,7 @@ runs: | |||
steps: | |||
- uses: dart-lang/setup-dart@v1 | |||
with: | |||
sdk: "${{ inputs.sdk }}" | |||
sdk: "${{ inputs.dart-sdk }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 I think this is a typo in the main
branch.
lib/src/logger/stderr.dart
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 Is there a better way to do this that still avoids Dart library 'dart:js_util' is not available on this platform
errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 Makes sense, and LGTM! 🚀
lib/src/node/console_stub.dart
Outdated
// 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 ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit silly, but it doesn't seem like you can conditionally import a package without also having a fallback package.
lib/src/logger/stderr.dart
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
WIP, but this gets us closer to fixing #25
self
/window
issues (Enable including libraries with a preamble in the browser directly mbullington/node_preamble.dart#29)process.*
compile()
,compileAsync()
,render()
, andrenderSync()
in the browsersass/sass-spec
repo. See Add support for running JS Spec Tests in browser context. sass-spec#1902