[Shared Resources] Add JS API tests by jerivas · Pull Request #1954 · sass/sass-spec

jerivas · 2023-11-14T21:47:21Z

Blocked until proposal is accepted.

[skip dart-sass]
[skip sass-embedded]

jerivas · 2023-11-14T21:51:08Z

I want to share resources across multiple invocations of the Sass compiler's JS API.

jerivas

I added some basic tests to verify the behavior described in the proposal. I think we should also run all the tests from compile.node.test.ts and compile.test.ts. What do you think of parametrizing these tests to accept both the standalone compile* functions as well as the compiler methods?

js-api-spec/compiler.test.ts

jgerigmeyer

👍

nex3

Actually, thinking about this more, it's probably a good idea to verify that the async compiler in particular gracefully handles multiple concurrent compilations, especially when those compilations are "chatty" (calling custom functions, importers, and loggers). Probably just one integration test case is sufficient as long as it handles a large number of chatty compilations at once and verifies that the right compilation calls get the right events.

Edit: I suppose we should also verify that at least two concurrent compilations work for the sync compiler, since it's theoretically possible to start a second one in a callback from the first.

js-api-spec/compiler.test.ts

jerivas · 2023-11-17T01:23:24Z

js-api-spec/compiler.test.ts

+      const result = compiler.compileString(
+        '@import "bar"; .fn {value: foo(baz)}',
+        {importers, functions, logger}
+      );


we should also verify that at least two concurrent compilations work for the sync compiler, since it's theoretically possible to start a second one in a callback from the first.

I'm not sure I 100% understand the request, but this is my attempt to do it by using importers and functions similar to the async case

I want to testing running multiple synchronous compilations at the same time. This means you'll need to start a second compilation within a callback for the first one.

Added in 7c17ae5

js-api-spec/compiler.node.test.ts

nex3 · 2023-12-02T00:00:36Z

js-api-spec/compiler.test.ts

+export const getSlowImporter = (callback: () => void) => ({
+  canonicalize: async () => new URL('foo:bar'),
+  load: async () => {
+    await new Promise(resolve => setTimeout(resolve, 100));


Rather than using a timeout when testing that concurrent systems wait for a given event, it's usually better to have a promise whose resolution you control explicitly so you can guarantee it won't finish until you want it to (and then you don't end up writing tests that take a lot longer than they need to because of delays). You can run await Promise(resolve => setTimeout(resolve, 0)) to force all outstanding non-time-based tasks to complete, then resolve the promise and verify that the behavior happens as expected.

Resolved in 6dcdd38

nex3 · 2023-12-02T00:01:29Z

js-api-spec/compiler.test.ts

+export const getLogger = () => ({debug: spy(() => {})});
+
+/* A slow importer that executes a callback after a delay */
+export const getSlowImporter = (callback: () => void) => ({


Declare functions as export function, and don't forget to include a return type.

Resolved in 6dcdd38

….shared-resources

nex3 · 2023-12-13T23:36:48Z

js-api-spec/compiler.node.test.ts

+          canonicalize: () => new URL('foo:bar'),
+          load: () => ({
+            contents: compiler.compile(dir('input-nested.scss')).css,
+            syntax: 'scss' as const,


Nit: Rather than as const, I think it's cleaner to add a type declaration to the variable itself (const nestedImporter: Importer) so that it infers the correct types for all its components. Similarly for the top-level fields in compiler.test.ts.

Updated in 0a14b6f.

nex3 · 2023-12-13T23:37:22Z

js-api-spec/compiler.node.test.ts

+
+describe('AsyncCompiler', () => {
+  let compiler: AsyncCompiler;
+  const runs = 1000; // Number of concurrent compilations to run


Declare this in the test where it's used

Moved in 0a14b6f

nex3 · 2023-12-15T01:30:09Z

js-api-spec/compiler.test.ts

+    canonicalize: (url: string) => new URL(`u:${url}`),
+    load: (url: typeof URL) => ({


I think now that importers itself is typed you don't need to type the parameters here (or in asyncImporters).

nex3 · 2023-12-15T01:30:39Z

js-api-spec/compiler.test.ts

+      expect(() => compiler.compileString('$a: b; c {d: $a}')).toThrowError();
+    });
+  });
+  it('errors if constructor invoked directly', () => {


Blank line before test case

Added in 76e676a

* main: Use lts/* and lts/-n for node version (sass#1957)

….shared-resources

Add basic compiler tests

b71d78f

jerivas mentioned this pull request Nov 14, 2023

[Shared Resources] Embedded host implementation sass/embedded-host-node#261

Merged

jerivas marked this pull request as draft November 14, 2023 21:49

jamesnw mentioned this pull request Nov 14, 2023

[Shared Resources] Add types and specs sass/sass#3744

Merged

Test for active compilations when disposing

e955667

jerivas commented Nov 14, 2023

View reviewed changes

js-api-spec/compiler.test.ts Outdated Show resolved Hide resolved

jerivas requested review from jamesnw and jgerigmeyer November 14, 2023 22:41

Fix "after disposed" tests

6145b4e

jerivas marked this pull request as ready for review November 15, 2023 19:09

jerivas requested a review from nex3 November 15, 2023 19:10

review

d1fe650

jgerigmeyer approved these changes Nov 15, 2023

View reviewed changes

nex3 approved these changes Nov 16, 2023

View reviewed changes

nex3 requested changes Nov 16, 2023

View reviewed changes

jamesnw reviewed Nov 16, 2023

View reviewed changes

js-api-spec/compiler.test.ts Outdated Show resolved Hide resolved

jamesnw reviewed Nov 16, 2023

View reviewed changes

js-api-spec/compiler.test.ts Outdated Show resolved Hide resolved

jerivas added 3 commits November 16, 2023 19:05

Ignore whitespace when comparing

89ef7e5

Remove extra wrapper fn

95fd558

More robust compiler tests

032e045

jerivas commented Nov 17, 2023

View reviewed changes

jerivas requested a review from nex3 November 17, 2023 01:23

jerivas added 2 commits November 21, 2023 01:29

Test more concurrent compilations

36db21b

Test compilations in callbacks

7c17ae5

jerivas mentioned this pull request Nov 21, 2023

[Shared Resources] dart-sass implementation sass/dart-sass#2134

Merged

jerivas added 2 commits November 22, 2023 00:12

Stricter tests for dispose()

386167f

Split node-specific compiler tests

2ac64d2

jamesnw reviewed Nov 27, 2023

View reviewed changes

js-api-spec/compiler.node.test.ts Outdated Show resolved Hide resolved

jamesnw reviewed Nov 27, 2023

View reviewed changes

js-api-spec/compiler.node.test.ts Outdated Show resolved Hide resolved

jerivas added 3 commits November 27, 2023 21:23

Address review

a2e6f0c

Lint

25562de

Increase timeout for slow CI

286e7fa

jerivas force-pushed the feature.shared-resources branch from 3834597 to 286e7fa Compare November 27, 2023 23:49

nex3 requested changes Dec 2, 2023

View reviewed changes

jamesnw added 2 commits December 11, 2023 11:09

Merge branch 'main' of https://github.com/sass/sass-spec into feature…

27dbeab

….shared-resources

Move from slowImporter to triggeredImporter

6dcdd38

jamesnw requested a review from nex3 December 12, 2023 17:44

nex3 requested changes Dec 13, 2023

View reviewed changes

Update types, localize run const

0a14b6f

jamesnw requested a review from nex3 December 14, 2023 15:19

Test compilers throw if constructed

0e3b3d9

nex3 requested changes Dec 15, 2023

View reviewed changes

jamesnw added 2 commits December 15, 2023 09:47

Update constructor tests

76e676a

Test compiler still works after a compilation failure

2730112

nex3 approved these changes Dec 15, 2023

View reviewed changes

jgerigmeyer added 3 commits January 8, 2024 13:39

Merge branch 'main' into feature.shared-resources

b1fec2d

* main: Use lts/* and lts/-n for node version (sass#1957)

review

60fc0f9

remove unnecessary types

b3f1e67

jgerigmeyer approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'main' of https://github.com/sass/sass-spec into feature…

6962fcb

….shared-resources

ntkme mentioned this pull request Jan 17, 2024

Provide a way to share resources across multiple compilations sass/sass#3296

Closed

nex3 merged commit f013971 into sass:main Jan 18, 2024

nex3 deleted the feature.shared-resources branch January 18, 2024 02:48

		canonicalize: (url: string) => new URL(`u:${url}`),
		load: (url: typeof URL) => ({

Conversation

jerivas commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerivas commented Nov 14, 2023

Uh oh!

jerivas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jgerigmeyer left a comment

Choose a reason for hiding this comment

Uh oh!

nex3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerivas Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jerivas commented Nov 14, 2023 •

edited

Loading

nex3 left a comment •

edited

Loading

jerivas Nov 17, 2023 •

edited

Loading