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

[javascript] Add support for parameters to JavaScript transformation #5128

Closed
thewilli opened this issue Mar 17, 2019 · 11 comments · Fixed by #10901
Closed

[javascript] Add support for parameters to JavaScript transformation #5128

thewilli opened this issue Mar 17, 2019 · 11 comments · Fixed by #10901
Labels
enhancement An enhancement or new feature for an existing add-on

Comments

@thewilli
Copy link

Currently - and due to the signature, the JavaScript transformation only works on the value and input script.

There are few usecases where scripts are similar but not identical. If there was a way to add parameters to them, we could prevent redundancy and maintain a single script file where few were needed otherwise. One example might be upper and lower bounds of an otherwise identical transformation.

In order to not break compatibility, I'd propose to support query string-like parameters as optional suffix to the script's file name.

Those values would for the sake of simplicity be always strings, but could be easily converted in the JS source of course. (In theory) it should not be more than parsing the query string, removing it from the filename and adding the values as additional bindings.

Example usage:

transform("JS", "my-script.js?a=foo&b=bar", value)

Number  Kitchen_Temperature    "Temperature [JS(temp.js?unit=F):%.1f °F]" {/* ... */}
@davidgraeff
Copy link
Member

Sounds like a good idea.

@wborn
Copy link
Member

wborn commented Mar 17, 2019

We've been using the "enhancement" label in issues and PRs for this "Feature Request" label @davidgraeff. Similarly to how "bug" is used in issues and PRs.

@wborn wborn added enhancement An enhancement or new feature for an existing add-on and removed Feature Request labels Mar 17, 2019
@wborn wborn changed the title [javascript][feature] add support for parameters to JavaScript transformation [javascript] Add support for parameters to JavaScript transformation Mar 17, 2019
@davidgraeff
Copy link
Member

davidgraeff commented Mar 17, 2019

@wborn I tried to write some descriptions for the labels, so that it is more obvious when to use a label. Maybe you can alter that to match what has been decided somewhere. I thought that "enhancement" is only used for PRs that "enhance" an extension.

@wborn
Copy link
Member

wborn commented Mar 17, 2019

Yes the descriptions will certainly help! The "enhancement" label is actually a default GitHub label so you can find it in many projects. See the list of default GitHub labels.

@davidgraeff
Copy link
Member

@thewilli Could you try to propose a PR to implement your idea?

@thewilli
Copy link
Author

@davidgraeff sure, though it depends if you expected me to create the (yet missing) unit tests for the JS transformation in order to extend them for the change 😉

@davidgraeff
Copy link
Member

If you are under 100 lines of code changes and comment well I guess I can live without unit tests.
Just make sure that you explain the feature in the readme file.

@thewilli
Copy link
Author

sounds good, thanks.

I just did some basic structuring.

@davidgraeff as for the change itself, would you agree that for the sake of simplicity and (runtime) efficiency

  • we can assume that if a ? is part of the filename, it is always considered a query string indicator.
    • this would prevent any scripts with a ? in their filename from being called
    • multiple occurencences would be considered an error
    • we would require a long and ugly regex otherwise
  • we don't do an extensive validation of the query string
    • if it can somehow be parsed, it will be used
    • if it is malformed some unexpected values could be used (file.js?name=alice&bob)

@davidgraeff
Copy link
Member

davidgraeff commented Mar 23, 2019

we can assume that if a ? is part of the filename, it is always considered a query string indicator.

Question marks are not allowed in url query strings and I guess we want to adhere to that standard. What you therefore can do is split the string at the last question mark and consider the last part as query. This way filenames with question marks are still fine.

we don't do an extensive validation of the query string

If the query string follows url escaping it can only contain ascii and must otherwise be escaped. You can therefore split at ampersands and get key=value entries. Entries not following that pattern would be considered an error. Values must be unescaped with java url decode means.

@thewilli
Copy link
Author

thewilli commented Mar 23, 2019

Question marks are not allowed in url query strings

sure, I was referring to how to detect the query string, and how the separation happens.

[...] split the string at the last question mark and consider the last part as query
This way filenames with question marks are still fine.

not necessarily, what about my?file, is it a filename or a query string. And even my?file=a is, depending on the file system, a valid name.

If the query string follows url escaping it can only contain ascii and must otherwise be escaped.
You can therefore split at ampersands and get key=value entries

myfile.js?a&&& does only contain chars valid according to RFC3986, and so would myfile.js?a&=b. I can and will handle many cases by using framework functionality, the question is if you want a proper validation or if framework errors would be sufficient.

@davidgraeff
Copy link
Member

Because the transformation is re-evaluated every time, I'd say we don't do a proper validation. The thrown exception should be enough hint to a user that his filenames are suboptimal.

I'm currently blocking the bundle to be migrated to the new buildsystem until you have proposed your PR.

#5229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants