Ensure bundling compatibility #779
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
hey @nex3, any chance you can review this? :) |
| }, | ||
| "dependencies": { | ||
| "chokidar": ">=2.0.0 <4.0.0" | ||
| }, | ||
| "main": "sass.dart.js", | ||
| "bin": "sass.js", | ||
| "keywords": ["style", "scss", "sass", "preprocessor", "css"] | ||
| "keywords": ["style", "scss", "sass", "preprocessor", "css"], | ||
| "browser": { |
There was a problem hiding this comment.
Can you explain what this field is doing? It doesn't seem to match npm's documentation for this field.
There was a problem hiding this comment.
https://github.com/defunctzombie/package-browser-field-spec
basically, it's a field used by bundlers to remap module requests inside a package (when bundling it for the web).
many projects use it, such as axios, react-dom, etc.
There was a problem hiding this comment.
Can you walk me through the use-case here? Dart Sass doesn't support running in the browser, so what benefit does this provide, exactly?
There was a problem hiding this comment.
I wanted to run render/renderSync inside a Web Worker, in order to compile .sass/scss code into .css.
With these changes I can bundle it and run it in the browser, so I'm not sure what you mean by "doesn't support".
There was a problem hiding this comment.
Dart Sass is built with the assumption that built-in Node.js modules will always be available. If they're not, it's liable to crash in unexpected ways if you invoke it wrong.
If the goal here is to invoke Dart Sass from within the browser, I want to do it right: by addressing #25 and not adding the default filesystem importer in a web context, and documenting that as an option.
There was a problem hiding this comment.
I understand wanting to provide a full built-in support for running sass with different hosts (other than node, i.e. deno or other runtimes).
Dart Sass is built with the assumption that built-in Node.js modules will always be available. If they're not, it's liable to crash in unexpected ways if you invoke it wrong.
Several node APIs (url, Buffer, process etc.) are polyfill-able and are being automatically polyfilled by bundlers out there. The "browser" field enables me to to ignore other ones (fs). As long as sass' process doesn't try to use that fs, nothing breaks.
My guess was that chokidar and readline are used for by the CLI, and so are not relevant to a browser-based flow.
In addition, I found that the existing data and importer options allow me to "connect" my custom (browser-compatible) fs and resolver to sass' process. This enables me to bypass the need for sass to require an fs provider and achieve rendering of .scss/sass files in the browser.
imho, this PR is a good first step towards resolving issue #25.
There was a problem hiding this comment.
Further investigation into @import handling revealed the gaps in terms of custom hosts. I now see what you mean.
|
@nex3 thx for the quick review. |
- declare "browser" field. webpack and other bundlers pick up this field and avoid bundling these deps. - upgrade node_preamble to 1.4.5 so that sass.dart.js bundle is Worker-compatible as well. checked locally. Removal of the self.require = require is still needed.
|
rebased to current |
|
Alright I'm closing until a more complete solution is achieved. |
upgrade node_preamble to 1.4.5 so that sass.dart.js bundle is Worker-compatible as well. checked locally. removal of theself.require = requireis still needed.