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

feat!: parameter object is null object #333

Merged
merged 5 commits into from
Sep 2, 2024

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Aug 28, 2023

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 28, 2023

tests fail because of tap issues...

Copy link
Contributor

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Cleaner and Simpler

new Function('paramsArray', `return { __proto__:null,${lines.join(',')}}`)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 29, 2023

Need to benchmark.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think this shoukd be a semver-major change.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 13, 2023

@climba03003

It seems that new NullObject is faster than using proto trick

node benchmark/null-object.js 
new NullObject() x 1,377,691,231 ops/sec ±0.11% (194 runs sampled)
{__proto__: null } x 33,097,458 ops/sec ±0.50% (185 runs sampled)
'use strict'

const { NullObject } = require('../lib/null-object')

const Benchmark = require('benchmark')
Benchmark.options.minSamples = 100

const suite = Benchmark.Suite()

suite.add(`new NullObject()`, function () {
  const result = new NullObject()
})
suite.add(`{__proto__: null }`, function () {
  const result = {__proto__: null }
})
suite
  .on('cycle', function (event) {
    console.log(String(event.target))
  })
  .on('complete', function () {
  })
  .run()

@mcollina
Copy link
Collaborator

Can you exclude windows v14 from CI?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 14, 2023

@mcollina done.

I would like to write another benchmark for the actual parameter generation. So please dont merge to fast.

I posted my benchmark to discuss if proto is the right way or not.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm looks nice!

@mcollina
Copy link
Collaborator

PTAL @ivan-tymoshenko

Copy link
Collaborator

@ivan-tymoshenko ivan-tymoshenko left a comment

Choose a reason for hiding this comment

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

lgtm

@ivan-tymoshenko
Copy link
Collaborator

@Uzlopak Can you post the bechmarks comparison pls?

@mcollina
Copy link
Collaborator

can you merge main?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 29, 2024

@mcollina done

Copy link
Contributor

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

We all missed this one.
cc @mcollina

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

LGTM

I have no power here - @Uzlopak could you edit the title with feat!: ... to stand out the breaking?

@Uzlopak Uzlopak changed the title feat: parameter object is null object feat!: parameter object is null object Aug 10, 2024
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 10, 2024

@Eomm

Donr

@mcollina mcollina merged commit eddab86 into delvedor:main Sep 2, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants