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.

Sign up for GitHub

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

Jump to bottom

Ensure bundling compatibility #779

Closed
wants to merge 4 commits into from
Closed

Ensure bundling compatibility #779

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bb93ee
Ensure bundling compatibility
AviVahl Jul 24, 2019
2ae6f4e
Travis: increase node version to 10
AviVahl Jul 24, 2019
7c794a2
Remove correct location
AviVahl Jul 24, 2019
015d6d5
Revert upgrade of node_preamble, per review
AviVahl Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion package/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,10 @@
},
"main": "sass.dart.js",
"bin": "sass.js",
"keywords": ["style", "scss", "sass", "preprocessor", "css"]
"keywords": ["style", "scss", "sass", "preprocessor", "css"],
"browser": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what this field is doing? It doesn't seem to match npm's documentation for this field.

Copy link
Author

@AviVahl AviVahl Jul 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further investigation into @import handling revealed the gaps in terms of custom hosts. I now see what you mean.

"fs": false,
"chokidar": false,
"readline": false
}
}