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

NodeJS run_ API method should return native JavaScript Promise #932

Conversation

devversion
Copy link

Currently the run_ method, exposed through the dart.sass.js file,
returns a Future. The Future object is not accessible due to the
Dart compilation process and Future not being a thing in the
JavaScript/NodeJS platform.

In order to provide a better interface to the Sass API for
JavaScript/NodeJS consumers, a native Promise is now returned.

Fixes bazelbuild/rules_sass#96.

Currently the `run_` method, exposed through the `dart.sass.js` file,
returns a `Future`. The `Future` object is not accessible due to the
Dart compilation process and `Future` not being a thing in the
JavaScript/NodeJS platform.

In order to provide a better interface to the Sass API for
JavaScript/NodeJS consumers, a native `Promise` is now returned.

Fixes bazelbuild/rules_sass#96.
@devversion devversion force-pushed the fix/node-return-javascript-native-promise branch from 6c7517d to 7699029 Compare January 15, 2020 11:04
@devversion
Copy link
Author

I tried figuring out if the test failures are due to changes in this PR, but it doesn't look like it. I sent an empty test PR to confirm this: #933

@jathak
Copy link
Member

jathak commented Jan 15, 2020

It looks like tests are failing because the CIs have started to use a version of the source_span package with slightly different output from our current specs. This should be resolved once #926 is merged.

@nex3
Copy link
Contributor

nex3 commented Jan 15, 2020

We're in the process of transitioning the way we build JS executables to use cli_pkg (#924), so it's probably a good idea to update this there rather than in this package.

@jelbourn
Copy link
Member

Would someone from the Sass team be able to follow up on that? The Angular folks can only go so deep into the dart ecosystem before we're over our heads

@nex3
Copy link
Contributor

nex3 commented Jan 16, 2020

I can, but it'll have to be after we do this migration. Can you file an issue on http://github.com/google/dart_cli_pkg?

@devversion
Copy link
Author

Just came across this again. I've created the issue on the dart_cli_pkg and also experimented a bit with it, but looks like it will be time consuming to set up the project/tests on my end: google/dart_cli_pkg#50

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

Successfully merging this pull request may close these issues.

Errors not properly reported in worker mode
4 participants