Skip to content
This repository has been archived by the owner on Aug 5, 2020. It is now read-only.

Replace function only replaces one inline script #6

Closed
wants to merge 1 commit into from

Conversation

wizardlyhel
Copy link
Contributor

descript.replace does not replace text across multiple inline scripts. This is a safety guard that is implemented in the descript replace function. We want to ensure there is no un-intentional inline script replacement.

descript.replace doesn't allow more than 1 search type. For example, the following will not work:

descript.replace({
    contains: [
        'google',
        'bing'
    ]
})

However, in the following example, we have the following behaviour:

There are 2 scripts that contains an ID you need to replace:

<script>
    ...
    var ID="123456789";
    var sID="123456789";
    ....
</script>
...
<script>
    ...
    var mob_ID="123456789"
    var tab_ID="123456789"
   ...
</script>

The following line will only replace all instance of the text of the second script and not the first script because descript works in reverse order.

descript.replace({contains: '123456789'}, /123456789/g,'987654321');

I am not sure to call this a feature or a bug. We are making sure that we are replacing only one inline script at a time. However, it is confusing not knowing that the first script did not have replace performed.

Question:
Should we make replace function perform replacement to all inline script instance found with the single search type?

Status: Opened for visibility

Reviewers: @donnielrt @fractaltheory @jansepar
JIRA: #5

Changes

  • (change1)

Notes

  • (note1)

@donnielrt
Copy link
Contributor

Hey, we’re looking to prune older abandoned PRs. If this PR is still relevant and you would like to see it merged in, please reopen the PR and we’ll add it to our backlog! Thanks!

@donnielrt donnielrt closed this Oct 9, 2015
@donnielrt donnielrt deleted the issue-5 branch October 9, 2015 23:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants