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

API: source-map related options enabling criteria #885

Closed
am11 opened this issue Feb 12, 2015 · 7 comments
Closed

API: source-map related options enabling criteria #885

am11 opened this issue Feb 12, 2015 · 7 comments

Comments

@am11
Copy link
Contributor

am11 commented Feb 12, 2015

With growing source-map related options, there has been some confusion among node-sass users.

node-sass takes the user options and send to LibSass as is (at the risk of default values undefined type casted to null).

I believe there is a need of a well-defined policy. to make the "require options to set" criteria for each case. I propose to make this kind of policy as relaxed as possible, i.e. let users set minimum options to enable the intended feature. To devise such a policy, some changes might be required.

Copying from sass/node-sass#622 (comment), here is an extract of what we have found:

  • source_map_contents == true should emit source-map without needing the presence of source_map.
  • source_map_contents == true should emit source-map without needing the presence of output_path.
  • source_map_embed == true should emit source-map without needing the presence of source_map.
  • source_map_embed == true should emit blank string on sass_context_get_source_map_string(ctx). (NEW based on Added support for sourcesContent and update to node-sass v2.0.1 joliss/broccoli-sass#43 (comment)).
  • source_map_embed == true should emit source-map without needing the presence of input_path, when data option is set.
  • source_map_embed == true && source_map_contents == true should emit source-map without needing the presence of output_path. (hint: emit file: '' in JSON)
  • source_map_embed == true && source_map_contents == true should emit source-map without needing the presence of input_path when data option is set.

(note: there might be more considerations here that I am missing ..)

Once agreed upon and LibSass API defines the policy to set which options to enable what, we will use the same in node-sass README (or hopefully wiki) for node-sass API implementers.

@simonexmachina
Copy link
Contributor

I can confirm that specifying sourceMapEmbed: true does not produce an embedded source map unless sourceMap: true is also specified. Is this expected behaviour or should this be fixed?

@xzyfer xzyfer added the C API label Feb 16, 2015
@mgreter
Copy link
Contributor

mgreter commented Mar 2, 2015

IMO this would mostly just mean to implicitly enable source_map when either source_map_contents or source_map_embed are enabled. IMO this can easily been done by implementors, but I don't have personaly anything against such a change. What's your opinion @xzyfer?

@xzyfer
Copy link
Contributor

xzyfer commented Mar 3, 2015

I agree with @am11, I believe I may have originally instigated this report. IMHO it's poor UX to have to enable source_map if you've explicitly declared which type of sourcemap you want.

Do we even need source_map? Is there a use case for declaring source_map that we cannot infer from other options?

@am11
Copy link
Contributor Author

am11 commented Mar 3, 2015

@xzyfer, yes that option is required: From libsass' API angle, sass_option_set_source_map_file essentially sets the string path. The option in node-sass (sourceMap) accepts my/preferred/path/to/file.map (String) as well as bool. If sets to true, node-sass resolves the sourceMap path with respect to outFile and pass it to libsass.

If source_map (string) is set and output_path is null, libsass can either:

  • assume output_path = change_extension(input_path, ".css");
  • or emit JSON with empty file: ''.

I prefer the latter one because it will not confuse the user and if someone comes to our shop with crazy use-case, we can easily pitch: what you set is what you get. 😄

@am11
Copy link
Contributor Author

am11 commented Mar 3, 2015

Also, neither libsass nor node-sass do anything with output_path, but both infer it as source_map_out_file. We can probably rename it as a breaking change, if we are chose to go with this option:

If source_map (string) is set and output_path is null, libsass will emit JSON with empty file:.''.

@mgreter mgreter added this to the 3.3 milestone Mar 9, 2015
@simonexmachina
Copy link
Contributor

Can I get clarification on my earlier question please: is it expected that specifying sourceMapEmbed: true does not produce an embedded source map unless sourceMap: true is also specified? I think this is unexpected and should be fixed, but let me know if this is the desired behaviour so consumers can handle it.

@mgreter
Copy link
Contributor

mgreter commented Sep 3, 2015

I have made sourceMapEmbed independent from source_map_file since they are just two different ways to "output" the source-map and the filename wasn't event used when embedded, so both options should enable the source-map generating!

@mgreter mgreter closed this as completed Sep 3, 2015
@mgreter mgreter modified the milestones: 3.3, 3.3.1 Sep 3, 2015
@mgreter mgreter self-assigned this Sep 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants