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

fix(minify): support minified client packages #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kdubb
Copy link

@kdubb kdubb commented Jul 7, 2020

The getArgumentNames function was fixed to resolve a few issues with minification as detailed below.

Whitespace

getArgumentNames tests for specific code pieces but assumes whitespace that may be removed by minification. The tests are changed to use regex’s that make the space optional.

class extend vs class

After minification code arises like the following:

class{constructor(e,t,r,n,i,a,o){this.id=e,this.aliases=t,this.caCertificate=r,this.hash=n,this.inception=i,this.deviceCount=a,this.data=o}}

Notice the lack of extends after class. This causes the class extends test to fail and causes a syntax error for the missing class name. This regex was altered to make the test for extends or the {, which causes replacement with class JacksonClass as required.

Connection with issue(s)

Resolve issue #7

Testing and Review Notes

All test pass.

To Do

Adding proper tests for this scenario requires minifying a client class that uses decorators from this package. Alternatively a number of strings of pre-minified code could be added, testing them against getArgumentNames directly.

For example:

const names = getArgumentNames('class{constructor(e,t,r,n,i,a,o){this.id=e,this.aliases=t,this.caCertificate=r,this.hash=n,this.inception=i,this.deviceCount=a,this.data=o}}')

expect(names).toEqual(['e','t','r','n','I','a','o']);

Which of these methods is best and should be implemented is a decision left for the package maintainer.

The `getArgumentNames` function was fixed to resolve a few issues with minification as detailed below.

### Whitespace
`getArgumentNames` tests for specific code pieces but assumes whitespace that may be removed by minification.  The tests are changed to use regex’s that make the space optional.

### `class extend` vs `class`
After minification code arises like the following:
```
class{constructor(e,t,r,n,i,a,o){this.id=e,this.aliases=t,this.caCertificate=r,this.hash=n,this.inception=i,this.deviceCount=a,this.data=o}}
```
Notice the lack of `extends` after `class`. This causes the `class extends` test to fail. This regex was altered to make the `extends` portion optional which causes replacement with `class JacksonClass`; as is required.
@kdubb
Copy link
Author

kdubb commented Jul 7, 2020

Note that this only fixes the Invalid keyword errors that arise after minification. Minification also renames parameters of functions so some method of manually specifying the parameter is still required.

Our code uses JsonProperty with explicit names (e.g. JsonProperty({ value: 'id' })) so this fixes our issues but this should be noted in the documentation or some method of recovering the argument names that works with minification should be implemented.

jingyu added a commit to elastos/jackson-js that referenced this pull request Jul 27, 2021
@nfonrose
Copy link

The fix in this PR solved our deserialization issues (which appeared when switching to a PROD configuration with minification) and this should definitively be merged.

In the meantime, we have to poll from (@outfoxx/jackson-js).

Thank you @kdubb for the fix.
And thank you @pichillilorenzo for jackson-js !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants