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

[PiranhaJS] Remove declaration of the identifier #92

Closed
mkr-plse opened this issue Aug 13, 2020 · 4 comments
Closed

[PiranhaJS] Remove declaration of the identifier #92

mkr-plse opened this issue Aug 13, 2020 · 4 comments
Labels
enhancement New feature or request legacy-js Issues/PR related to older PiranhaJS implementation

Comments

@mkr-plse
Copy link
Contributor

Is it planned for Piranha to support refactoring/removing the declaration of the identifier argument?
e.g. removing all of the code in this block, instead of just the conditional block:

const featureFlag = "feature-flag"
if (isFlagTreated(featureFlag)) {
 // do stuff
}

If we had that functionality, we could likely make the assumption that the configured API argument index always refers to a string literal and expect the flag name to be a string literal node. Being able to search across multiple files/imports would likely be required for some of the more complex usages of feature flags

Originally posted by @atrakh in #91 (comment)

@mkr-plse mkr-plse added enhancement New feature or request good first issue Good for newcomers legacy-js Issues/PR related to older PiranhaJS implementation labels Aug 13, 2020
@anag004
Copy link
Collaborator

anag004 commented Aug 13, 2020

Not sure if this is relevant, but we might need to make PiranhaJS aware of variable scope here. Without this, the following code

{ var featureFlag = "feature1-flag"; } // scope 1

// ...

{ var featureFlag = "feature2-flag"; } // scope 2

will be refactored incorrectly when cleaning up one of the flags. For example, when cleaning up "feature1-flag", featureFlag will be replaced by "feature1-flag" everywhere.

@msridhar
Copy link
Collaborator

The escope library can be helpful for variable scopes:

https://github.com/estools/escope

@atrakh
Copy link
Contributor

atrakh commented Aug 13, 2020

Not sure if this is relevant, but we might need to make PiranhaJS aware of variable scope here.

Good point... this is some of the magic I hand-waved away 😄

@mkr-plse mkr-plse removed the good first issue Good for newcomers label Aug 13, 2020
@ketkarameya
Copy link
Collaborator

@atrakh we can now perform this kind of cleanup in polyglot piranha.
However, on-boarding JS of Polyglot Piranha is pending.
I am closing this issue for now, since polyglot piranha allows developers to support for such cleanups.
Feel free to reopen this issue if you have further questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request legacy-js Issues/PR related to older PiranhaJS implementation
Projects
None yet
Development

No branches or pull requests

5 participants