JPeer264 / node-rcs-core

Rename css selectors across all files
MIT License
37 stars 7 forks source link

Selector matching a HTML's node shouldn't be replaced in querySelector #93

Closed X-Ryl669 closed 5 years ago

X-Ryl669 commented 5 years ago

Example of failing code:

<p class='b'>zurg</p>
<div id="zorg">It is <span></span>, for <b><b> days</div>
  var el = document.getElementById('zorg'); 
  el.querySelector('span').textContent = time;
  el.querySelector('b').textContent = days;

Here, the 'b' selector is understood as some class by rcs and not as a HTML's element name. I wonder if it wouldn't be better to improve the JS parser in order to assign a flag to each string if it's inside an unknown string, or inside a function expecting a selector (like querySelector and querySelectorsAll)

In the later case, only valid CSS selectors will be changed (that is, with a . or # prefix).

This won't break code like:

  var t = "someclass";
  var el = document.querySelector('.' + t);

since the former StringLiteral will be replaced as usual.

This will still break code like (like currently):

   var t = "b";
   var el = document.querySelector(t);

Another solution is to reserve all HTML's possible element name (like a, b, i, q, s, u, ul, etc...) and exclude any mapping using them. That's probably safer but will limit the possible compression ratio.

What do you think ?

JPeer264 commented 5 years ago

Hm ya I get you, that might be a problem. I think that would be nice to ignore querySelector where there is no identifier. For things like your example var t = 'b'; we could save those variables in a kind of map somewhere and access it later on.

Saving those variables would be just when node.type === 'Literal' || node.type === 'TemplateElement'. If that's the case we could save it into { [parent.id.name]: node.value }. TemplateElements might be a bit a problem, as they could have nested variables as well.

Furthermore, we need a list where functions like querySelector can be extended, as many libraries might have the same principle.

Another solution is to reserve all HTML's possible element That would be an idea, but I would just have it as option for setExcludes or something. setExcludeDefaults({ ignoreHtmlLike: boolean }) or maybe as normal option? Default's to true I'd say.

X-Ryl669 commented 5 years ago

For things like your example var t = 'b'; we could save those variables in a kind of map somewhere and access it later on.

Complexity will kill us here. One could do var t = 'b'; t += 'c'; t += someFunc(); el.querySelector(t). Not sure it's a good solution anyway, sounds good at first by after thinking about this, it's too complex to implement and test correctly.

I wonder if we could add a specific "tag" to let rcs knows we are expecting only selectors here instead. Something like rcs('#some .selector b .here'). We only check for a function called rcs that's simply implemented as function rcs(text) { return text; }, everytimes we find this function, we know that we must replace the parameter by selectors.

Or, simply do the opposite, instead of searching non prefixed selectors in javascript (like here instead of .here), only accept prefixed selectors, except for getElementById and getElementsByClassName. Again code complexity could break here (if one is doing var t = 'something'; document.getElementById(t))

Arrgggg. No solution is good. The only solution I think that could work would be to instrument the user code (let it run in a monitored environment) and capture all calls to querySelector et al and then track the source of the arguments, but that's a huge work.

I guess excluding the HTML tag name from renaming will be good enough for now. I did this for my project and it's working. So I'll do like you said.