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

Custom resolver #174

Closed
dgp1130 opened this issue May 7, 2022 · 13 comments
Closed

Custom resolver #174

dgp1130 opened this issue May 7, 2022 · 13 comments
Labels
enhancement New feature or request

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented May 7, 2022

Hi, I'm trying to use @parcel/css to bundle CSS files, however there doesn't seem to be a way to provide a custom resolver for @import statements, the package appears to directly use the real file system. This is a challenge for my use case where I'm hoping to use Parcel for CSS bundling in a Bazel workspace. Bazel is a build tool which puts generated files in different directories from the source code, meaning that @import './foo.css' could resolve to ./foo or $project_root/dist/bin/path/to/dir/foo.css, or even a few other paths. I would like to configure @parcel/css to resolve these paths correctly, but AFAICT, this package doesn't have any direct support for that. See dgp1130/rules_prerender#46 and dgp1130/rules_prerender@7e995d6 for a little more info about the use case and my attempt to implement it.

It does seem that custom resolvers are supported in .parcelrc, but I don't get the impression @parcel/css has any understanding of that configuration file. I'm unclear how the parcel package supports custom CSS resolvers if @parcel/css doesn't, but maybe they don't compose each other in the way I think they do? If there is a more appropriate package or different level of abstraction I should be using which would support resolvers, please let me know and I'm happy to try that.

If it does make sense to support custom resolvers in @parcel/css, I'm happy to put in the effort to contribute it. I think we would just need to update BundleConfig to accept a SourceResolver from JS and then use that instead of the FileResolver. All the inputs and outputs would be strings, so serializing into and out-of JS shouldn't be too much of an issue. I think it's possible, though I haven't done too much with Rust or WASM, so any pointers or suggestions here would be appreciated.

Is this a contribution you would be interested in accepting or am I going about this problem in the wrong manner?

@devongovett
Copy link
Member

You should use Parcel for this eg via the api https://parceljs.org/features/parcel-api/. Parcel CSS is used under the hood, and you can hook into resolution via the plugin system. Standalone, Parcel CSS has very limited bundling support.

@dgp1130
Copy link
Contributor Author

dgp1130 commented May 8, 2022

@devongovett, thanks for the suggestion. I spent most of today trying to get things working with the Parcel API. I was kind of able to get a custom resolver working, but it turned out much more complicated than I expected. Parcel plugins seem to require being loaded from an NPM package, with some tricks available to load local plugins. However as a build system, Bazel is already capable of solving those orchestration challenges pretty easily, and Parcel's tight coupling with NPM and node_modules/ actively fights against it. The .parcelrc file and plugin packages are providing features I don't need with Bazel, which was why I was trying to aim for a lower level of abstraction in @parcel/css.

I'm wondering if there's a package under @parcel/* which provides the same hooks as the Parcel API but without all that configuration and loading? The Parcel API seems too high level (lots of automated loading and importing which doesn't work well in a Bazel context with a local plugin) while @parcel/css seems to be too low-level (can't specify a custom resolver). What I would want to do is something like:

import { Parcel, getDefaultParcelConfig, getDefaultParcelResolver } from '@parcel/something';
import { Resolver } from '@parcel/plugin';

// Create a custom resolver dynamically.
function createResolver(context: any): Resolver {
  return new Resolver({
    async resolve() { /* ... */ }
  });
}

const bundler = new Parcel({
  entries: 'input.css',
  // Pass in a config object directly, no need to automatically import anything.
  config: {
    ...getDefaultParcelConfig(),
    resolvers: [ createResolver(someUseCaseSpecificContext), getDefaultParcelResolver() ],
  },
});

const bundled = await bundler.bundle();
await fs.writeFile('output.css', bundled);

This is a bit idealized, and passing through the default configuration is a bit awkward, but I think this structure would remove a lot of the coupling between Parcel and node_modules/, making the system much more flexible for use cases outside that structure.

Does an API at this level of abstraction exist in Parcel? Or is the Parcel API the closest thing?

@devongovett
Copy link
Member

I think you're looking for something similar to #82, though perhaps rather than providing the file contents you only need to do some kind of custom mapping of the filenames? We could add a JS API for that, but I was worried that it would impact performance since we'd need to block on the JS thread for each dependency.

When you use Parcel, the bundle API from @parcel/css is not currently used. In that case, each file is processed independently with the analyzeDependencies option, and Parcel uses the resulting information to perform bundling independently. Eventually we will probably migrate parts of that over too, but as described above, there are some challenges performance-wise.

If you're willing to write Rust, you could do this today using a custom SourceProvider wrapping around FileProvider. The Bundler API accepts an arbitrary SourceProvider and uses it to perform all filesystem operations.

@dgp1130
Copy link
Contributor Author

dgp1130 commented May 10, 2022

Ah, I didn't realize this module was also published as a crate. I'm certainly open to writing Rust here and can explore that option.

I see your concern with the performance, making a JS call for every import is definitely a hit and I understand why you would want to avoid it. My immediate thoughts are:

  1. This feature shouldn't slow down anyone who isn't using a custom resolver, so you only pay for the feature if you really need it.
  2. If a user really wants a custom resolver without the performance hit, they could always implement it at the Rust level like you're suggesting.
  3. For my particular use case I could actually pre-compute a map of all available imports and where they actually are on the filesystem. This is the way I would implement the resolver anyways. The Rust API could just accept this map as an input and look it up in Rust to avoid the JS callback. That's kind of specific to my use case though, so I understand if that's too limiting for what you think Parcel should support.

#82 does seem very similar. My use case is still on the filesystem, just at different paths, so a resolver alone would be good enough for me. However, I understand that providing file contents directly would probably be more flexible, and I'm fine making the fs.readFile() call if necessary. I'm not sure what the performance implications of that would be though, I'm sure it's more significant than the JS resolver function call, but maybe that's just the cost of doing what I want to do here?

@dgp1130
Copy link
Contributor Author

dgp1130 commented May 11, 2022

I took a pass at a Rust implementation and it seems to mostly work, it's definitely much easier to integrate with Bazel than using the Parcel API since I no longer need to deal with getting the configuration file and resolver to load. The two pieces of feedback I have from this experiment are:

  1. The parcel_css crate is still in alpha (1.0.0-alpha.24) as of now. Are there plans to get to a 1.0.0 release? Is this crate considered public API of Parcel?
  2. SourceProvider is designed to accept only a single path, and the bundler actually joins the current path with the imported path, assuming that they are typical file paths where that makes sense. In Bazel web tooling (rules_nodejs), absolute imports are typically written as workspace_name/path/to/foo with the workspace_name/ prefix signifying an absolute path (relative paths would start with ./). This scheme doesn't work with the bundler as is today, for example:
/* my_workspace/foo/bar.css */

/* Absolute import of `test.css` from the project root. */
@import 'my_workspace/test.css';

This results in a call to the SourceProvider with the path: my_workspace/foo/my_workspace/test.css. I get that the conventions are a little weird in this regard, but it's supposed to align with a Node resolve, since my_workspace/... wouldn't be a relative import in Node, but instead look for the my_workspace NPM package, which Bazel resolves to the whole workspace.

I think the issue here is that the bundler is assuming that is ok to join the import specifier with the current file path, when that may not be true or have the intended effect. Ideally SourceProvider would have two different fields, one for the file making the import, and another for the raw import specifier.

pub trait SourceProvider: Send + Sync {
    fn read<'a>(&'a self, import_specifier: &str, imported_from_file: &Path) -> Result<&'a str>;
}

This would also allow plugins to treat specifiers as non-file paths. For example, if someone wanted to implement @import 'assets!./foo.css', they would have the freedom to parse and interpret the specifier in whatever way makes sense for their use case.

Personally I would still love to see a JS binding for path resolution, though Rust is good enough as a workaround. Sorry that this issue is covering a lot of different topics, just trying to share my experience with configuring Parcel for an uncommon use case and the challenges I encounter. Hopefully this is useful to see where it can be improved. I'm happy to take a stab at implementing some of these suggestions if you think they are the right way to go here.

@devongovett
Copy link
Member

The parcel_css crate is still in alpha (1.0.0-alpha.24) as of now. Are there plans to get to a 1.0.0 release? Is this crate considered public API of Parcel?

The Parcel CSS Rust API has a massive surface area because it exposes structs for each individual CSS rule/property/value. The alpha status indicates that these structs are still changing as we work on improvements/features. However, the quality of the parsing/compiling/minifying itself (the parts exposed to JS) is considered stable, and it should be safe to use top-level APIs like StyleSheet and Bundler.

Ideally SourceProvider would have two different fields

I think probably we'd have a separate method on the SourceProvider trait to resolve the path since read is called for both entries and @import rules, whereas resolve would only need to be called for @import. We could have a default implementation that does source_path.with_file_name(specifier), as we do today.

Personally I would still love to see a JS binding for path resolution

Happy to also consider that, but getting the Rust implementation going first would be a good start.

dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
…r` (parcel-bundler#174)

This allows the `fs!` macro to be reused with other `SourceProvider` implementations on a per-test basis.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
…r` (parcel-bundler#174)

This allows the `fs!` macro to be reused with other `SourceProvider` implementations on a per-test basis.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
…l-bundler#174)

This allows each caller to define its own assertions and not be forced into asserting for `BundleErrorKind::UnsupportedLayerCombination`.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
This adds a `resolve()` method to `SourceProvider` which is responsible for converting an `@import` specifier to a file path. The default `FileProvider` still uses the existing behavior of assuming the specifier to be a relative path and joining it with the originating file's path.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
…r` (parcel-bundler#174)

This allows the `fs!` macro to be reused with other `SourceProvider` implementations on a per-test basis.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
…l-bundler#174)

This allows each caller to define its own assertions and not be forced into asserting for `BundleErrorKind::UnsupportedLayerCombination`.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue May 13, 2022
This adds a `resolve()` method to `SourceProvider` which is responsible for converting an `@import` specifier to a file path. The default `FileProvider` still uses the existing behavior of assuming the specifier to be a relative path and joining it with the originating file's path.
@dgp1130
Copy link
Contributor Author

dgp1130 commented May 13, 2022

Thanks @devongovett! I made #177 with an initial attempt at adding a resolve() method to SourceProvider.

devongovett pushed a commit that referenced this issue May 17, 2022
* Refactor bundle tests' `fs!` macro to drop dependency on `TestProvider` (#174)

This allows the `fs!` macro to be reused with other `SourceProvider` implementations on a per-test basis.

* Refactor `error_test()` to invoke a callback for its assertion (#174)

This allows each caller to define its own assertions and not be forced into asserting for `BundleErrorKind::UnsupportedLayerCombination`.

* Add `resolve()` method to `SourceProvider` (#174)

This adds a `resolve()` method to `SourceProvider` which is responsible for converting an `@import` specifier to a file path. The default `FileProvider` still uses the existing behavior of assuming the specifier to be a relative path and joining it with the originating file's path.
@dgp1130
Copy link
Contributor Author

dgp1130 commented May 21, 2022

I spent some time today trying to implement a JS resolver and wanted to document my findings so far (apologies for the wall of text, I just want to get this down somewhere before I forget). Again, I'm not that familiar with Rust or NAPI, so take everything here with a grain of salt.

TL;DR:

  1. Executing JS from Rust inherently pulls on multithreading issues.
  2. Multithreading issues inherently pull on async issues.
  3. To be async we either need to make css.bundle() async, or significantly overcomplicate the job model for background threads.
  4. Making css.bundle() async sounds like the easier approach to me, though I haven't actually tried to do it yet.

I was hoping to get an API like:

const { code } = css.bundle({
  filename: 'styles.css',
  resolver: {
    async resolve(specifier: string, originatingFile: string): Promise<string> {
      return path.join(specifier, originatingFile);
    },

    async read(filePath: string): Promise<string> {
      return await fs.readFile(filePath, 'utf-8');
    },
  },
});

If either resolve() or read() was undefined, the Rust side should fall back to their default implementations. Async sounded hard, so I ignored the Promise types to start with and just did everything synchronously.

The first problem I ran into was that updating the BundleConfig with a new pub read: Option<Box<dyn Fn(String) -> String>> doesn't work because Serde can't derive Deserialize. I get that closures generally can't be serialized/deserialized to strings, but this is coming from a JS context, so I'm unclear what "deserialize" really means here. I don't think it's passing through a string format, but maybe I'm missing something?

I worked around this by just manually getting napi::JsFunction values off the options napi::JsObject and invoking them manually. Not elegant, but good enough.

#[cfg(not(target_arch = "wasm32"))]
#[js_function(1)]
fn bundle(ctx: CallContext) -> napi::Result<JsUnknown> {
  let opts = ctx.get::<JsObject>(0)?;

  let resolver = opts.get::<_, napi::JsObject>("resolver")?.unwrap();
  let read = opts.get::<_, napi::JsFunction>("read")?.unwrap();

  // ...
}

I then tried to stick the napi::JsFunction values into a new SourceProvider implementation but found that didn't work because SourceProvider implements Send + Sync and napi::JsFunction isn't thread safe. This is what I mean by "Executing JS from Rust inherently pulls on multithreading issues". I discovered napi::threadsafe_function::ThreadSafeFunction and I tried switching to that. After a lot of trial and error, I was able to construct a thread safe function from the input and successfully invoke it, but I found that the JS read() function never received the call (a console.error() line never printed).

After a lot more research, I eventually came to understand the ThreadSafeFunction API (one useful reference) in that it doesn't actually invoke the function, only queues it. Later on, the main thread is responsible for checking this queue, converting the arguments to JS via the callback, and then invoking the function. This model presents a couple challenges:

  1. The ThreadSafeFunction API presents no means of receiving a result from the invoked JS function. I think the only option here would be to pass in another Rust callback function which gets invoked by the JS when it is completed. A small JS shim in css.bundle() could make this fit the desired API.
  2. The main thread has to be free to actually invoke any JS functions.

That second issue is particularly tricky to work around and was the reason the JS wasn't actually receiving the call, because the main thread was blocked and not able to process the JS execution queue. Since css.bundle() is synchronous (doesn't return a Promise), there isn't really an opportunity for the main thread to be free during Rust execution. I found that adding

await new Promise((resolve) => setTimeout(resolve, 1_000));

to the end of the test case yielded the main thread to the JS event queue and gave Rust an opportunity to actually execute the queued JS and read() actually received the invocation. This is what I mean by "multithreading issues inherently pull on async issues". Calling JS requires the main thread and the main thread is blocked in a synchronous execution model. So the easiest solution here would probably be to make css.bundle() async, this would actually be necessary to support async read() and resolve() functions, and given the file system access that's happening under the hood, I don't think it's unreasonable to do this, but I can understand wanting to avoid it.

In theory, css.bundle() doesn't need to be async as long as the read() and resolve() callbacks aren't async. However, since ThreadSafeFunction can't be invoked off the main thread and can't directly return a value this gets quite complicated. It seems like the core multithreading is happening via Rayon with parallel iterators. Since these threads can't keep a reference to the SourceProvider, they would need to send a message to the main thread to run the read() / resolve() operations for them and then send the results back. This is particularly difficult because the main thread is blocked on the worker threads. In theory, the main thread doesn't need to be hard blocked this way and is free to do other work synchronously in css.bundle(), though you'd need some awkward synchronization mechanisms to make that work. One example using spin locks could look something like this (using TypeScript-like pseudo-code because I'm not fluent enough with Rust):

// Queue of paths to resolve.
const resolveQueue = new ThreadSafeQueue<[ threadId: number, specifier: string ]>();

// Queue of resolved paths.
const resolvedQueue = new ThreadSafeQueue<[ threadId: number, path: string ]>();

function bundle(entryPoint: string, sourceProvider: SourceProvider): string {
  const stylesheet = parseStylesheet(entryPoint);

  // Spawn a thread for each rule for simplicity.
  const threads = stylesheet.rules.map((rule) => thread.spawn((threadId) => {
    if (!rule.isImport()) return; // Ignore other rule types for simplicity.

    // Queue a request for the main thread to resolve the given import.
    resolveQueue.push([ threadId, rule.importUrl ]);

    // Spin lock until the main thread has resolved the import.
    let path: string;
    while (true) {
      const resolved = resolvedQueue.find(([ tid ]) => threadId === tid);
      if (!resolved) continue;
      path = resolved[1];
    }

    // Use resolved `path`...
  }));
    
   // Spin lock on the main thread until all background threads are complete.
  while (!threads.every((thread) => thread.isDone())) {
    if (resolveQueue.length === 0) continue; // Nothing to do.

    // Take an item from the queue, call the JS `resolve()` function, and queue the result back.
    const [ [ threadId, specifier ] ] = resolveQueue.splice(0, 1);
    const path = sourceProvider.resolve(specifier); // Probably need a callback here since `ThreadSafeFunction` can't return directly.
    resolvedQueue.push([ threadId, path ]);
  }

  // All threads completed, do something with the result...
}

That's theoretically possible, but sounds like a mess to manage and I'm doubtful that something like Rayon has utilities to manage this kind of multithreading model. There might be a cleaner way to do this with other multithreading primitives. If you haven't noticed, I'm a frontend dev by trade, so I'm not used to thinking in the multithreading mindset or familiar with the modern tools in this space.

I do think the easier option is to make css.bundle() async and replace all read() and resolve() calls to accept a Future. This is desirable anyways since it allows the JS read() and resolve() implementations to be async, which is necessary to avoid synchronous JS file I/O. I don't know much about Rust's async runtimes, but I think that would also give an opportunity for Rust I/O to be async as well and get some better thread utilization (no need to block the whole thread on one file read), but I don't know how hard it would be to realize that gain.

If you want to keep introduce a separate css.bundleAsync() call, distinct from css.bundle() and sacrifice the custom resolver feature for synchronous execution, then maintaining a single codebase which supports both use cases might be quite tricky (can't just use Future since that would break the synchronous version). Given that the async version is theoretically faster due to non-blocking I/O, maybe it would be better to move that direction anyways? I don't know what other plans you have for @parcel/css in this respect.

@devongovett, I think the core question here is: How do you feel about making css.bundle() async (or having an async version)? Does that seem like a viable approach here? Does it align with the general direction of @parcel/css?

@devongovett
Copy link
Member

@dgp1130 thanks for your detailed analysis! Did you look into the blocking mode for napi's thread safe function API? I haven't used it before, but looks like there is ThreadsafeFunctionCallMode::Blocking, which should cause the calling thread to block until the JS thread returns if the queue size is set to 1. Maybe that's simpler than making everything async?

As for getting the return value, I think it should be possible, but we might need help from napi-rs. In the underlying C API, the thread safe function actually calls a C callback on the main thread, and not the JS function directly. The C callback's responsibility is then to call the JS function however it needs. napi-rs handles this internally, and currently doesn't expose a way to get the return value, but I think it could be updated to do so. cc. @Brooooooklyn.

We could make everything async, but as you noted, it would add a significant amount of complexity. Previously I have found that async I/O actually might be slower when you have multiple worker threads of your own anyway (under the hood, node is just maintaining a thread pool to do the I/O), but could be worth experimenting with. The issues about the main thread blocking until the rayon tasks are complete sound harder to address, so it might be necessary. In that case, we should introduce a new async version of the API rather than making a breaking change I guess.

@dgp1130
Copy link
Contributor Author

dgp1130 commented May 23, 2022

@devongovett, thanks for looking this over. I did use ThreadsafeFunctionCallMode::Blocking, and my interpretation of that feature was whether or not it should block when adding to the call queue, and that using NonBlocking just meant there was a chance adding to the queue could fail and you could get a QueueFull result. I think Blocking meant "wait until the queue can accept the call" rather than "wait until the call is made". But, I can't find the documentation which stated that now, but the C doc link you used mentions this behavior as well.

Looking at the C API for napi_call_threadsafe_function(), I don't see an obvious place to receive a return value from the invoked JS function? It seems to mostly mirror the Rust implementation. Are you saying that the native callback function is responsible for manually invoking the JS function on the main thread and then handling the result in whatever way it wants to? I can see how that would make sense with this API structure, though that's not my reading of the docs given that napi_call_threadsafe_function() accepts a napi_threadsafe_function which is defined as "an opaque pointer that represents a JavaScript function". Maybe that JS function value could be wrapped, but in a C context, I'm not sure how you would decorate a napi_threadsafe_function to do some native work before and after? After some quick Googling, I couldn't find an example of how to receive a return value from a JS function invocation in N-API, but also didn't find a definitive answer that it was impossible.

That's a good point about how Node async I/O is just another thread pool, I had always hoped there was some native API which passed those performance improvements down the whole stack, but I don't think it actually works that way. I can try an approach which introduces a new css.bundleAsync() function and implements everything with futures, making the JS call back into Rust. I don't know if Rust has any good utilities for doing "possibly async" work so we can share the same bundle implementation in a way that is asynchronous when needed and synchronous when not. I guess since Future is just a polling mechanism, maybe it can be implemented with Future and then the sync version would just do an immediate poll and assert that the result is present? I don't know enough about Rust async to know if that makes any sense, but it sounds possible? If anyone has ideas or suggestions in that regard, it would really help me out.

One other thing I was thinking about was that I noticed a lot of #[cfg(not(target_arch = "wasm32"))]. I always assumed Parcel executed its Rust in WebAssembly, but I guess if it's using N-API that's not true? Is there a WebAssembly build where we'll run into the same problem and have to devise a similar solution, or does Parcel only support N-API execution?

@devongovett
Copy link
Member

Looking at the C API for napi_call_threadsafe_function(), I don't see an obvious place to receive a return value from the invoked JS function

You'd need to marshal it back I guess. Here's the implementation in napi-rs: https://github.com/napi-rs/napi-rs/blob/main/crates/napi/src/threadsafe_function.rs#L400. Right now it passes a null pointer to the return value and doesn't do anything with it, but I guess it could and then attach it to the data argument so that the result could be passed back. But if you're right and blocking doesn't actually mean blocking, then maybe that's not possible. Would have to try it I guess.

Is there a WebAssembly build where we'll run into the same problem and have to devise a similar solution, or does Parcel only support N-API execution?

There is a WASM build, but it does not currently support the bundle API because WASM doesn't have file system access.

dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

The two `SourceProvider` methods are now `async` which will give a hook for JavaScript to implement them and be called from the main thread. Everything is `async` now where possible and `bundle()` executes the `Future` synchronously to maintain its contract. Since it won't support custom JavaScript resolvers, there should never be a case where `bundle()` can't execute synchronously.

Rayon doesn't seem to support `async` iterators, but also shouldn't be as necessary here. Removed it in order to call `async` APIs.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This is roughly equivalent to `bundle()`, except that is executes the `Future` *asynchronously*, returning a JS `Promise` holding the result.

Errors are formatted a bit differently since they just use the `Display` trait to convert to strings, rather than the `throw()` function which converts them to a JS `SyntaxError` in `bundle()`. This can't be used in the async version because it must return a `napi::Result` which is immediately emitted to the user and no JS values are accessible. While it is possible to return the `CompileError` directly from the async block in a `napi::Result<Result<TransformResult, CompileError>>` and then call `throw()` in the callback with access to JS data, doing so causes lifetime issues with `fs` and isn't easily viable.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…#174)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function for JS to invoke with a result. `napi-rs` also requires Node async calling conventions, so the actual contract of these functions are:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(err: any, file: string, cb: (string) => void): void {
      if (err) cb(err, null); // Propagate error.

      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(err: any, specifier: string, originatingFile: string, cb: (string) => void): void {
      if (err) cb(err, null); // Propagate error.

      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that N-API sends an `error` as the first parameter and a `callback` as the last parameter, expecting the function to propagate errors and invoke the callback. This is unergonomic, so this shim presents a traditional async API on top of these implementation details.

Also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function for JS to invoke with a result. `napi-rs` also requires Node async calling conventions, so the actual contract of these functions are:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(err: any, file: string, cb: (string) => void): void {
      if (err) cb(err, null); // Propagate error.

      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(err: any, specifier: string, originatingFile: string, cb: (string) => void): void {
      if (err) cb(err, null); // Propagate error.

      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that N-API sends an `error` as the first parameter and a `callback` as the last parameter, expecting the function to propagate errors and invoke the callback. This is unergonomic, so this shim presents a traditional async API on top of these implementation details.

Also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function using Node calling conventions (`callback(err, result)`). This looks like:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(file: string, cb: (string) => void): void {
      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(specifier: string, originatingFile: string, cb: (string) => void): void {
      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function using Node calling conventions (`callback(err, result)`). This looks like:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(file: string, cb: (string) => void): void {
      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(specifier: string, originatingFile: string, cb: (string) => void): void {
      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

The two `SourceProvider` methods are now `async` which will give a hook for JavaScript to implement them and be called from the main thread. Everything is `async` now where possible and `bundle()` executes the `Future` synchronously to maintain its contract. Since it won't support custom JavaScript resolvers, there should never be a case where `bundle()` can't execute synchronously.

Rayon doesn't seem to support `async` iterators, but also shouldn't be as necessary here. Removed it in order to call `async` APIs.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This is roughly equivalent to `bundle()`, except that is executes the `Future` *asynchronously*, returning a JS `Promise` holding the result.

Errors are formatted a bit differently since they just use the `Display` trait to convert to strings, rather than the `throw()` function which converts them to a JS `SyntaxError` in `bundle()`. This can't be used in the async version because it must return a `napi::Result` which is immediately emitted to the user and no JS values are accessible. While it is possible to return the `CompileError` directly from the async block in a `napi::Result<Result<TransformResult, CompileError>>` and then call `throw()` in the callback with access to JS data, doing so causes lifetime issues with `fs` and isn't easily viable.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function using Node calling conventions (`callback(err, result)`). This looks like:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(file: string, cb: (string) => void): void {
      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(specifier: string, originatingFile: string, cb: (string) => void): void {
      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 6, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
@dgp1130
Copy link
Contributor Author

dgp1130 commented Jun 6, 2022

Proposed implementation of custom JS resolver in #196. Mostly works, but still a few kinks to work out.

dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
)

The two `SourceProvider` methods are now `async` which will give a hook for JavaScript to implement them and be called from the main thread. Everything is `async` now where possible and `bundle()` executes the `Future` synchronously to maintain its contract. Since it won't support custom JavaScript resolvers, there should never be a case where `bundle()` can't execute synchronously.

Rayon doesn't seem to support `async` iterators, but also shouldn't be as necessary here. Removed it in order to call `async` APIs.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
This is roughly equivalent to `bundle()`, except that is executes the `Future` *asynchronously*, returning a JS `Promise` holding the result.

Errors are formatted a bit differently since they just use the `Display` trait to convert to strings, rather than the `throw()` function which converts them to a JS `SyntaxError` in `bundle()`. This can't be used in the async version because it must return a `napi::Result` which is immediately emitted to the user and no JS values are accessible. While it is possible to return the `CompileError` directly from the async block in a `napi::Result<Result<TransformResult, CompileError>>` and then call `throw()` in the callback with access to JS data, doing so causes lifetime issues with `fs` and isn't easily viable.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function using Node calling conventions (`callback(err, result)`). This looks like:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(file: string, cb: (string) => void): void {
      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(specifier: string, originatingFile: string, cb: (string) => void): void {
      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
)

The two `SourceProvider` methods are now `async` which will give a hook for JavaScript to implement them and be called from the main thread. Everything is `async` now where possible and `bundle()` executes the `Future` synchronously to maintain its contract. Since it won't support custom JavaScript resolvers, there should never be a case where `bundle()` can't execute synchronously.

Rayon doesn't seem to support `async` iterators, but also shouldn't be as necessary here. Removed it in order to call `async` APIs.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
This is roughly equivalent to `bundle()`, except that is executes the `Future` *asynchronously*, returning a JS `Promise` holding the result.

Errors are formatted a bit differently since they just use the `Display` trait to convert to strings, rather than the `throw()` function which converts them to a JS `SyntaxError` in `bundle()`. This can't be used in the async version because it must return a `napi::Result` which is immediately emitted to the user and no JS values are accessible. While it is possible to return the `CompileError` directly from the async block in a `napi::Result<Result<TransformResult, CompileError>>` and then call `throw()` in the callback with access to JS data, doing so causes lifetime issues with `fs` and isn't easily viable.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
)

This adds a new `JsSourceProvider` which acts a `SourceProvider` which invokes associated JS implementations of the resolver if they exist or falls back to `FileProvider` when not given. This allows JS consumers to override one or both of these methods. If JS does *not* override either, they should not pay any significant performance penalty since all the Rust work will stay on the same thread.

The JS implementations are invoked as thread-safe functions, pushing the arguments onto a queue and adding it to the event queue. Some time later, the main thread pulls from this queue and invokes the function. `napi-rs` doesn't seem to provide any means of receiving a JS return value in Rust, so instead arguments for both `read()` and `resolve()` include a callback function using Node calling conventions (`callback(err, result)`). This looks like:

```javascript
await bundleAsync({
  // Options...

  resolver: {
    read(file: string, cb: (string) => void): void {
      // Read file and invoke callback with result.
      fs.read(file, 'utf-8').then((res) => cb(null, res), (err) => cb(err, null));
    },

    resolve(specifier: string, originatingFile: string, cb: (string) => void): void {
      // Resolve and invoke callback with result.
      const resolved = path.resolve(originatingFile, '..', specifier);
      cb(null, resolved);
    },
  },
});
```
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
…(...args) => Promise<Result>` (parcel-bundler#174)

This hides the fact that `napi-rs` doesn't actually do anything with the return value of a threadsafe JS function, so any returned data would be dropped. Instead, Parcel adds a callback function which gets invoked by JS with the resulting value, following Node conventions of `callback(err, result)`. This is unergonimic as an API, so the JS shim exposes a `Promise`-based interface which gets converted to the callback behavior required by `napi-rs`.

This also includes tests for all the common use cases and error conditions of custom JS resolvers.
dgp1130 added a commit to dgp1130/parcel-css that referenced this issue Jun 25, 2022
This rejects immediately if the user attempts to pass a `resolver` property to the synchronous `bundle()` function. Since communciating across between background threads and the main thread is quite tricky without an asynchronous hook, `bundle()` does not support custom resolvers.
@mischnic mischnic added the enhancement New feature or request label Jul 20, 2022
@devongovett
Copy link
Member

#263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants