RCS replaces all words that are the same as the selectors #20

Stephen-X · 2018-08-21T18:04:11Z

It's part of a series of issues I found, so I'm going to reuse the base test case and open separate issues for each of them 😄 I'm developing on Windows btw; not sure if these are all platform specific bugs.

Issue Description:

RCS replaces words that are the same as selectors in the wrong elements (e.g. <meta>, <p>).

Steps to reproduce this issue:

Create a simple project with the following code:

index.html:

<html>
    <head>
        <meta name="description" content="Fire in the hole!">
        <link rel="stylesheet" href="style.css">
    </head>
    <body div class="in wf-active">
        <p>Fire in the hole!</p>
        <script src="script.js"></script>
    </body>
</html>

style.css:

.wf-active {
    color: red;
}

.in {
    font-weight: bold;
}

script.js:

console.log('Fire in the hole!');

selector_obfs.js:

const rcs = require('rename-css-selectors')

rcs.loadMapping('./temp/renaming_map.json')

rcs.processCss('style.css', {newPath: 'temp'}, err => {
    rcs.process(['script.js', 'index.html'], {newPath: 'temp'}, err => {
        rcs.generateMapping('./temp', { overwrite: true }, err => {
            // the mapping file is now saved
        })
    })
})

Run RCS by node selector_obfs.js. The "in" word in script.js file got renamed:

console.log('Fire n the hole!');

As well as <meta> in index.html's <head>:

<meta name="description" content="Fire n the hole!">

When I was working on my own projects, I found that words in <p> could get renamed as well, although for some reason I couldn't reproduce it in this example.

The text was updated successfully, but these errors were encountered:

JPeer264 · 2018-08-25T08:37:10Z

First of all the HTML problem is resolved by updating rcs-core (ref: JPeer264/node-rcs-core#52 JPeer264/node-rcs-core#56)

The JavaScript problem is following. I haven't found a good way of determining if a string is used for selectors or just as text. I came up with detecting non word characters in the text (in regex \W) and if some got found no renaming happens on this text. You might have another or additional idea to this problem?

Stephen-X · 2018-08-26T03:42:51Z

Hi @JPeer264 thanks for following up! I think a simple way to deal with the JS problem would be to only find and replace word matches with a prefix of "." or "#" in all strings. Do you think that's sufficient enough?

JPeer264 · 2018-08-26T06:47:47Z

Hm no that wouldn't be enough, as e.g. bootstrap is storing the selectors as plain strings sometimes: bootstrap.js#L405-L409, or jQuery has functions such as addClass, where selectors don't have any prefixes.

Stephen-X · 2018-08-26T13:17:49Z

Hmm you're right, there's no easy way to distinguish class names accurately without missing something or overkilling. There're three more complicated ways I can think of that may work though:

Allow users to wrap all CSS classes in JS and HTML inside something like css() during development, and they'll be automatically removed, selectors being renamed or not (since some of them might be excluded), during output. A simple regex of css\((.*?)\) should do the job. A downside of this method is that it adds RCS as a dependency, but we can probably add an -remove-css-wrapper option that simply outputs JS and HTML with css() wrapper removed so that those who no longer require RCS in their projects can sanitize their codes.
Make replacing an (optional) interactive process. RCS will act as greedy as possible (simply treat JS and HTML as a list of strings and try to find and replace all matches), and users will be able to confirm each replacement. This works better on small projects.
RCS will act as conservative as possible (as stated previously, it will only search for matches that have a prefix of "." or "#"), but users can add an include section in .rcsrc where they can put names of functions and objects in which they require replacing all occurrences.

JPeer264 · 2018-08-26T14:06:48Z

Ya, those are good approaches and already had the same or similar approaches. In total I am more a fan of automation and less configuration. Here are my thoughts:

This was the first idea I came up with in the beginning of the project. There is already a similar way in rcs to achieve such a behavior. The mapping file was the solution. As the mapping file is a JSON you can simply add following in your code:
```
const map = require('./mapping.json'); // or anything to load this JSON in browsers
const myClass = map['.my-selector'];
```
But the downside of this is, that it doesn't really save disk space, which is actually my goal of this lib.

The downside of your approach (what I think) is, that the users (probably me as well) might forget about the wrapper and then the entire app won't work as expected. Which is also the same case with the mapping file.
This would be one of the better solutions, but I am a big fan of automation and having an interactive process might be really annoying over some time, especially when the code changes a lot. Which might be the case on smaller projects.
I think this would work the best with a low missing rate. But this is also a lot of configuration, especially when having a lot of objects and functions.

Stephen-X · 2018-08-26T14:19:31Z

I took a look at other selector obfuscation solutions such as Google Closure Tools, it would seem that it enforces users to mark CSS classes with goog.getCssName(): https://github.com/google/closure-stylesheets#renaming, https://github.com/google/closure-stylesheets/wiki/More-on-CSS-Renaming, so yeah, option 1 seems like a preferred solution: user should clearly mark CSS class names that they want RCS to process, otherwise RCS should just ignore them. This is a bit cumbersome (I agree that I myself might forget about the wrapper sometimes), but I think this might be the best balance between automation and code safety (it would be easier to debug the generated code by checking if you forgot to wrap a class name somewhere). The current solution (just replace all occurrences) is simply not safe enough to be used in production environment, as it could create unexpected outcomes that are hard to spot (such as changing the display content).

I'm not sure the use of mapping file is a good solution though. For starters, as you mentioned, the JS code will then contain a lot of redundant codes. The reference to the automatically generated mapping.json adds an unnecessary and fragile dependency to the source code, and will also add to the number of round trips made by browsers.

JPeer264 · 2018-08-26T14:44:47Z

I totally agree that it is risky in production.

I guess this could work as you suggested in 1., replacing the code of rcs.css() with the selectors. In HTML however, just class and id are triggered, so adding those functions in JS are enough I guess.

However, your added documentation of the closure tools gave me an idea. I would like to add options for triggering classes in JS (so if I use another library, such as bootstrap, I am still able to rename those classes in an experimental way). I think following options for replacing selectors in JS would be neat:

experimental - the current way
closure (or similar) default - your suggested way of replacing all rcs.css(), without prefix # or ., or rcs.cssClass() | rcs.cssId(), which adds the prefix

I guess that works right?

JPeer264 · 2018-08-26T14:47:19Z

Btw, the original issue with your HTML should be fixed in v3.0.0

Stephen-X · 2018-08-26T14:58:17Z

It's possible that if devs develop their own elements for styling, those won't be renamed? But yeah, only triggered by class and id seems enough already, at least for me :)

Adding an indirection layer like the Google Closure Tools did does seem like a better solution! In this way the css() marker is no longer just a piece of invalid marker code, and the original source code can run with RCS import. They can all be removed in the final output.

JPeer264 · 2018-08-26T15:17:59Z

No those elements are not yet removed. But this is actually a good feature. As I use Parse5 for HTML and PostCSS for AST it is not a big deal. Custom elements in HTML are easy to find, as it is well structured and reliable (maybe I am also missing something).

Totally, I am also happy with that solution. It will just take some time I guess until this is implemented, as I also have to think about a good structure.

klimashkin · 2018-08-31T00:04:19Z

With css-modules I don't have such problem, because in my case all class names are generated in that way '[name]_[local]', like Button_primary.

If you want to add some css('classname'), make it optional please, because currently everything works fine for me : )

JPeer264 · 2018-08-31T17:13:30Z

@klimashkin good point. Then it is also not a breaking change 👍

JPeer264 · 2018-09-10T06:52:43Z

Hey @Stephen-X .. I won't have much time the next few weeks. The implementation of this feature will take some time, I hope you'll understand.

Stephen-X · 2018-09-15T05:38:36Z

@JPeer264 No worries, take your time. I mostly use this utilities for personal projects 😄 Thanks for the great works so far!

emilemil1 · 2018-12-23T22:13:02Z

Would it be safe to assume that if a string (after concatenation) contains any word that isn't a CSS selector, then every word in that string should be excluded from renaming? I can't personally think of any situation where you pair selector names with other random text, except possibly in some error description.

That would still miss a lot of cases, but it would solve the initial example and quite a few others.

JPeer264 · 2018-12-25T10:35:53Z

I am not sure if I understood it correctly, but I assume you meant something like following: Let's say we have two string in JavaScript, one with CSS selectors and one with normal string:

const myString = 'Any string of my selector';
const mySelectors = 'selector another-selector';

And I have my CSS files:

.selector {}

.another-selector {}

In the variable myString there are Any, string, of and my included which are not defined in my CSS, so therefor this whole string can be excluded from renaming (which means selector in myString won't get minified, according to the other words).

If this is what you meant, then what about selectors which are not defined in any CSS, something like "JavaScript only selectors", such as #js-slider or .js-triggered-by-javascript. Then these would get trigger the logic when a whole string won't get minified, right?

emilemil1 · 2018-12-25T13:39:04Z

Indeed, that's a destructive case I didn't consider. The aforementioned manual workarounds of adding a prefix/wrapper or similar would be needed to mark any JS-only selectors.

X-Ryl669 · 2019-06-24T20:57:56Z

For me, in JS you shouldn't deal with CSS's class or ID everywhere. Typically, it'll boils down to querySelector/getElementByID/getElementsByTagName/addClass/removeClass/toggleClass...

The list is somehow sparse. Unluckily, you need to compute the AST for the complete JS script to figure out if whatever string is being used for such methods. I wonder if you can compile the complete JS code (with all modules), compile to AST, track the methods using some string, and mark/flag such strings for renaming (using the source map ?).
Obviously, code like var sel = 'mySelector'+i; whatever.toggleClass(sel); might not be renamed correctly, but it'll be detected as a candidate (sel is a variable that's used by toggleClass that's been modified in the AST). In that case, the code could (either):

Easier: Fill the exclusion list with mySelector (so it's not renamed), and output a warning on the console.
Create a new unmapped.json file storing the source line where such construct is detected and let the developer change his code.
Harder: Make sure the mapping is done in 2 steps (mySelector prefix is renamed to abc but anything after mySelector is kept unmodified, so a css rule .mySelector0 or .mySelector_down is renamed to .abc0 or .abc_down)

JPeer264 · 2019-07-01T19:57:25Z

The only bad thing is that there are a lot of modules which needs selectors, so everytime a new module is imported, which needs selectors, it must be included into some list of available "replaceable" functions.

Those combined selectors are really hard to detect, as I already mentioned in here: JPeer264/node-rcs-core#85 (comment)

This might be the best solution I guess. You mean to output a console.log when there would be a potential selector?
That would be also a good hint to tell the developer such things are happening
Hm I think that cannot work like this as I don't know if the prefix is the correct class or the appendix. So it could also be that some developers make it the other way arround: .0mySelector or .down_mySelector. In this case I would rather disallow every change where such cases appear and would go for 1. and 2.

X-Ryl669 · 2019-08-19T21:21:14Z

I guess with all the changes in rcs-core#95, this should be somehow fixed. At least, I had the same issues before and now it's working correctly on my project which is fairly large with many tricks (like Template, Id/Class mismatch, HTML using <b> and <i>, JS code making string from ID + iterator and so on.

Hopefully, you'll be able to test soon!

waterplea · 2020-04-10T14:53:21Z

Just tried this tool and ran into similar issue. I have error class used in my html, in js I have a constant error.password.invalid as an error code from backend. I get error in JS renamed :(

JPeer264 · 2020-04-10T17:12:45Z

@waterplea which version do you use?

JPeer264 · 2020-04-10T17:14:46Z

You could also exclude error globally for such things:

import rcs from 'rcs-core';

// following methods also allow array of strings and RegExp
rcs.selectorLibrary.setExclude('error'); 
// or rcs.selectorsLibrary.setExclude (if you use rcs-core@3+)

waterplea · 2020-04-10T17:33:57Z

I used the latest. Thanks for the suggestion, in my case I ended up with no need to process js, so excluded it.

JPeer264 · 2020-04-10T17:36:15Z

Alright. I the next version 4+ a lot of bugs will be fixed. Yours should be fixed as well. To be sure I will add a test case specific to your problem ;)

waterplea · 2020-04-22T12:34:23Z

Hi @JPeer264, I've found another error. Now I have this case - I used this error key in data attribute and it got minified:
<div data-error="error.password.invalid">
And in CSS I have [data-error="error.password.invalid"]:before and it got minified to [data-error="error.ts.to"]. Your suggestion helped but it seems like even though I use 3+ and typings suggest selectorsLibrary with "S" - actual code is already without "S" on 3.3.0

JPeer264 · 2020-04-22T17:39:11Z

Alright, thanks for the update, I will add this to my tests.

I think it is kinda confusing right now with the versions. Following should be true:

rename-css-selectors version: 3.3.0
rcs-core version: 2.6.3

I think you have installed [email protected]

I guess it will be better for the future to have a monorepo and namespace everything, that all versions match across the packages; but this is now another topic.

heychazza · 2020-06-23T08:53:56Z

I'm having

Hi @JPeer264, I've found another error. Now I have this case - I used this error key in data attribute and it got minified:
<div data-error="error.password.invalid">
And in CSS I have [data-error="error.password.invalid"]:before and it got minified to [data-error="error.ts.to"]. Your suggestion helped but it seems like even though I use 3+ and typings suggest selectorsLibrary with "S" - actual code is already without "S" on 3.3.0

Having a similar issue, but with EJS tags <%= some.thing %> any way to exclude these with regex even?

JPeer264 added the bug label Aug 25, 2018

JPeer264 self-assigned this Aug 25, 2018

JPeer264 added the enhancement label Sep 10, 2018

This was referenced Jan 25, 2019

Some strings was changed incorrectly #36

Closed

Some strings was changed incorrectly JPeer264/gulp-rcs#5

Closed

JPeer264 mentioned this issue Apr 10, 2020

Add new test case JPeer264/node-rcs-core#115

Open

JPeer264 mentioned this issue Jun 28, 2020

[BUG] It renames EJS #64

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCS replaces all words that are the same as the selectors #20

RCS replaces all words that are the same as the selectors #20

Stephen-X commented Aug 21, 2018 •

edited

Loading

JPeer264 commented Aug 25, 2018

Stephen-X commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018 •

edited

Loading

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018 •

edited

Loading

JPeer264 commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

klimashkin commented Aug 31, 2018 •

edited

Loading

JPeer264 commented Aug 31, 2018

JPeer264 commented Sep 10, 2018

Stephen-X commented Sep 15, 2018

emilemil1 commented Dec 23, 2018 •

edited

Loading

JPeer264 commented Dec 25, 2018

emilemil1 commented Dec 25, 2018

X-Ryl669 commented Jun 24, 2019

JPeer264 commented Jul 1, 2019

X-Ryl669 commented Aug 19, 2019

waterplea commented Apr 10, 2020 •

edited

Loading

JPeer264 commented Apr 10, 2020

JPeer264 commented Apr 10, 2020 •

edited

Loading

waterplea commented Apr 10, 2020

JPeer264 commented Apr 10, 2020

waterplea commented Apr 22, 2020

JPeer264 commented Apr 22, 2020

heychazza commented Jun 23, 2020

RCS replaces all words that are the same as the selectors #20

RCS replaces all words that are the same as the selectors #20

Comments

Stephen-X commented Aug 21, 2018 • edited Loading

Issue Description:

Steps to reproduce this issue:

JPeer264 commented Aug 25, 2018

Stephen-X commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018 • edited Loading

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018 • edited Loading

JPeer264 commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

Stephen-X commented Aug 26, 2018

JPeer264 commented Aug 26, 2018

klimashkin commented Aug 31, 2018 • edited Loading

JPeer264 commented Aug 31, 2018

JPeer264 commented Sep 10, 2018

Stephen-X commented Sep 15, 2018

emilemil1 commented Dec 23, 2018 • edited Loading

JPeer264 commented Dec 25, 2018

emilemil1 commented Dec 25, 2018

X-Ryl669 commented Jun 24, 2019

JPeer264 commented Jul 1, 2019

X-Ryl669 commented Aug 19, 2019

waterplea commented Apr 10, 2020 • edited Loading

JPeer264 commented Apr 10, 2020

JPeer264 commented Apr 10, 2020 • edited Loading

waterplea commented Apr 10, 2020

JPeer264 commented Apr 10, 2020

waterplea commented Apr 22, 2020

JPeer264 commented Apr 22, 2020

heychazza commented Jun 23, 2020

Stephen-X commented Aug 21, 2018 •

edited

Loading

Stephen-X commented Aug 26, 2018 •

edited

Loading

Stephen-X commented Aug 26, 2018 •

edited

Loading

klimashkin commented Aug 31, 2018 •

edited

Loading

emilemil1 commented Dec 23, 2018 •

edited

Loading

waterplea commented Apr 10, 2020 •

edited

Loading

JPeer264 commented Apr 10, 2020 •

edited

Loading