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

Argument 'maxAge' must be false or a number #1133

Closed
oskarrough opened this Issue Oct 29, 2018 · 13 comments

Comments

2 participants
@oskarrough
Copy link

oskarrough commented Oct 29, 2018

Whenever I minify the following example, remotestorage.js throws an error:

import RemoteStorage from 'remotestoragejs'
const remoteStorage = new RemoteStorage({logging: true})
remoteStorage.access.claim('foo', 'rw')

const client = remoteStorage.scope('/foo/')
client.getListing('', false).then(listing => console.log(listing))
// error: "Argument 'maxAge' must be false or a number"

Note, this only happens after building the code. If you don't include remotestorage in the bundle but use a global via cdn or so, there is no error. I am not connecting to any remote storage, e.g. only local.

Could it be how the code is minified? You can test by putting above example inside a create-react-app (webpack) or npx bili example.js --format umd (rollup).

May be related to something here https://github.com/remotestorage/remotestorage.js/blob/master/src/syncedgetputdelete.js#L25

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

Seems to be me like it might be stripping the empty string argument (which it shouldn't do). Have you tried with a non-empty string? Like so e.g.:

client.getListing('/', false)
@oskarrough

This comment has been minimized.

Copy link

oskarrough commented Oct 30, 2018

It minifies that line into client.getListing('/', !1). Seems ok? If you leave the second argument out does remotestorage.js default to false anyway?

I put the example here to better inspect :)

https://csb-2op6w034zj-lnloqmcccy.now.sh/
https://codesandbox.io/s/2op6w034zj

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

That's some hardcore minification. Apparently the rs.js argument validation is not happy with that.

If you don't provide an argument for maxAge it uses the default value (2x the sync interval) to see if it should check for changes on the remote beforehand, or serve the listing from cache directly. So if you want it to always return from cache directly you need to provide the argument in some way.

See https://remotestoragejs.readthedocs.io/en/latest/js-api/base-client.html?highlight=getListing#getListing for details (also see the linked explanation of maxAge and caching behavior).

In order to fix your problem without patching rs.js, you could e.g. provide the argument as a very high number of milliseconds instead.

And in order to fix the problem upstream and for other people using a minifier like that, you could see if you can change the argument validation so it accepts !1 instead of false.

@skddc skddc added the bug label Oct 30, 2018

@oskarrough

This comment has been minimized.

Copy link

oskarrough commented Oct 30, 2018

Thanks for explanation!

I don't think the argument validation is the issue, because both false and -1 pass the test.

function maxAgeInvalid(maxAge) {
return maxAge !== false && typeof(maxAge) !== 'number';
}

maxAgeInvalid(false) === maxAgeInvalid(-1) // true`

BUT, if I replace remotestorage.js from the minified bundle with a <script src="https://cdn.jsdelivr.net/npm/remotestoragejs/"></script> tag, the minified code works.

So maybe the real issue is that the create-react-app minifier modifies the imported remotestorage.js in a way it shouldn't?

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

-1 is a number. !1 is an expression.

Edit: Hmm, both work for me. So yeah, could be that it messes with the library in a different way.

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

I checked your example on now.sh, but I couldn't see any errors in the console logs (FF stable).

Edit: I do see the error in Chromium. Checking if it's just because it's an uncaught promise error...
Edit 2: My dev tools are disappointing me now, and I can't even get the debugger back to the index.js that I saw before. It seems like the deployed app might have changed while I was looking at it. In any case, it's kinda hard to debug production code like that.

@oskarrough

This comment has been minimized.

Copy link

oskarrough commented Oct 30, 2018

Sorry, confused the -1 and !1 ⚡️I also don't see any errors in Firefox, only Chrome. The source code is in the other link --> https://codesandbox.io/s/2op6w034zj.

But now I'm getting curious so created another maybe simpler test repo here:
https://github.com/oskarrough/rsmaybebugtest

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

Haha, wow! Here's what your minifier does to the original code that receives the maxAge internally (t being maxAge), in slightly more readable form than a single line:

get: function (e, t) {
  return this.local ? (
    void 0 === t && ('object' === n(this.remote) && this.remote.connected && this.remote.online ? t = 2 * this.getSyncInterval()  : (o('Not setting default maxAge, because remote is offline or not connected'), t = !1)),
    Promise.reject('Argument \'maxAge\' must be false or a number')
  ) : this.remote.get(e)
},

As you can see, it doesn't even contain the actual local.get() call anymore, and mashes everything else together as much as possible. Or I guess I should say more than is actually possible there. Because some important parts are missing, and others are just false compared to the original code. (By the way, that comma before the promise rejection is a comma operator.)

We do ship our own minified versions of the library and didn't run into this before. It seems that whatever minifier and config create-react-app is using is trying a bit too hard, and as a result failing miserably with this function.

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 30, 2018

@oskarrough Could you try using 4ca0f0c as dependency and see if it helps? I refactored the source code a little bit, so hopefully the minifier doesn't destroy it anymore.

@oskarrough

This comment has been minimized.

Copy link

oskarrough commented Oct 31, 2018

@skddc tried but couldn't see any difference, sorry.

Continuing this rabbit hole: if I run webpack --mode production on the test repo above it also fails. BUT, if you disable the uglifyjs.compress option, the error no longer occurs and the remotestorage test returns the listing as usual.

I'm wondering what causes the minifier to break here...
Should !true be a valid maxAge?`

I also tried with the terser minifier, but same error 🤷‍♂️

skddc added a commit that referenced this issue Oct 31, 2018

Refactor synced GET a little bit
Simplify the code. Aside from being more readable, this fixes a problem
with the UglifyJS compressor destroying the code by restructuring it
wrong.

closes #1133

@wafflebot wafflebot bot added the in progress label Oct 31, 2018

skddc added a commit that referenced this issue Oct 31, 2018

Refactor synced GET a little bit
Simplify the code. Aside from being more readable, this fixes a problem
with the UglifyJS compressor destroying the code by restructuring it
wrong.

closes #1133
@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 31, 2018

When looking at the output I noticed that your app didn't actually use my updated code, because it's importing from the release build, but I failed to commit that to my branch. However, with those changes, it was still destroying the code, albeit in a slightly different way.

I have now changed it again (#1134), and have arrived at a version that actually works after being compressed to death by uglifyjs.

I'm still rather perplexed about how uglify's compression algos just straight-up destroy perfectly fine JavaScript code by restructuring it completely wrong. So I think this should actually be fixed over there.

But fwiw, at least our code is a bit more readable now, and hopefully the entire build now works with the default settings that started this issue. I wouldn't actually bet on the latter though, because this was by no means a complex part of our code in comparison to the rest of the library.

@oskarrough

This comment has been minimized.

Copy link

oskarrough commented Nov 1, 2018

Hey, I tried your branch and I can confirm it also works here now! Thank you very much.

Did you learn exactly which code/syntax uglifyjs failed with? Just curious.

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Nov 1, 2018

Glad to hear.

Did you learn exactly which code/syntax uglifyjs failed with? Just curious.

Yes, both the code I originally linked, as well as the version from my linked refactoring commit (4ca0f0c).

@skddc skddc closed this in #1135 Nov 1, 2018

@wafflebot wafflebot bot removed the in progress label Nov 1, 2018

DougReeder added a commit to DougReeder/remotestorage.js that referenced this issue Nov 18, 2018

Refactor synced GET a little bit
Simplify the code. Aside from being more readable, this fixes a problem
with the UglifyJS compressor destroying the code by restructuring it
wrong.

closes remotestorage#1133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment