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

test: update user-agents data, update dev deps #32

Merged

Conversation

antongolub
Copy link
Contributor

Maintenance PR to make sure the library supports newly added user agents

test.js Outdated
Comment on lines 64 to 69
const ua = deviceCategory =>
ua[deviceCategory] ||
(ua[deviceCategory] = new UserAgent([
({ userAgent }) => userAgent !== exclude,
{ deviceCategory }
]))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to cache the UserAgent instances here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm having trouble looking up the UserAgent API, do you know where its repo is located now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just follows the guide: https://github.com/intoli/user-agents#useragentrandom. random() generates new instances too, but also reuses filtered data preset and so that's «over 100x faster than the initial construction».

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry I thought we were using user-agent not user-agents

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the tests too slow without this optimization?

Copy link
Contributor Author

@antongolub antongolub Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can simplify this piece a bit to make it clearer. I'm a nerd. I do optimizations by default)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

premature optimization is the root of a lot of evil :P

@juliangruber
Copy link
Owner

Thank you for submitting this PR!

test.js Outdated
// user-agents v1.0.843
const exclude =
'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.198 Safari/537.36'
ua[deviceCategory] = new UserAgent([
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is still caching the user agent instance 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It caches the data preset which is attached to the instance. There's no other way to bind filtered values: setCumulativeWeightIndexPairs declared as module scope variable.
https://github.com/intoli/user-agents/blob/master/src/user-agent.js#L140

 random = () => {
    const userAgent = new UserAgent();
    setCumulativeWeightIndexPairs(userAgent, this.cumulativeWeightIndexPairs);
    userAgent.randomize();
    return userAgent;
  };

Authors literally suggest using this approach:
https://github.com/intoli/user-agents#useragentrandom

// Use the `random()` method to construct a second user agent.
const firstUserAgent = new UserAgent(filters);
const secondUserAgent = firstUserAgent.random();

image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against using .random(), I'm against caching UserAgent instances in ua[...]

Copy link
Contributor Author

@antongolub antongolub Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just noticed new Proxy inside the UserAgent constructor. Optimizations don't make any sense)

Copy link
Owner

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, thanks for removing the caching on the ua function, why did you also remove .random()? I thought that was good to add?

@antongolub
Copy link
Contributor Author

random is just a constructor-copy-like factory exposed as a proto method. It requires a ua reference to bind its this.cumulativeWeightIndexPairs to new ones.

@antongolub
Copy link
Contributor Author

Finally, I've replaced ua factory memoization with an in-iterator scope variable.

test.js Outdated
const checks = [
['mobile', true],
['tablet', true, { tablet: true }],
['desktop', false]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I think just one last change, could you refactor this data structure to be an array of objects, like [{ family: 'mobile', tablet: false, ...}]? This way it's easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np

@juliangruber juliangruber merged commit 9aa57fd into juliangruber:master Nov 24, 2021
@juliangruber
Copy link
Owner

perfect, thank you so much!

@antongolub antongolub deleted the update-user-agents-data branch November 24, 2021 11:51
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.

None yet

2 participants