core: export any page-functions as string #5902
Conversation
midzer
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
|
thanks @midzer! @paulirish was specifically requesting this too :) what do we think about adding a suffix to make it clear they are strings? I suppose the danger is less since toString'ing the string won't do much harm 🤷♀️
|
|
@patrickhulce thanks for your suggestions. I'm totally up for this change. While looking a bit through LH code base, I've realized there are a couple of situations where variable type is put into it's name like for example. For me this resembles to https://en.wikipedia.org/wiki/Hungarian_notation with just a bit different flavour. Dunno whether there exists an exact term for this notation. In rare cases something like So I'm unsure which way to go to keep code base consistent for this kind of topic. |
| checkTimeSinceLastLongTask: checkTimeSinceLastLongTask.toString(), | ||
| getElementsInDocument: getElementsInDocument.toString(), | ||
| getOuterHTMLSnippet: getOuterHTMLSnippet.toString(), | ||
| ultradumbBenchmark: ultradumbBenchmark.toString(), |
There was a problem hiding this comment.
one wrinkle here we actually use this function as a function in scripts/benchmark.js
It's not super consistent throughout the codebase, so I wouldn't put it on yourself to make it so ;) In the file that a few of these functions were just moved from they were labeled |
paulirish
changed the title
| checkTimeSinceLastLongTaskString: checkTimeSinceLastLongTask.toString(), | ||
| getElementsInDocumentString: getElementsInDocument.toString(), | ||
| getOuterHTMLSnippetString: getOuterHTMLSnippet.toString(), | ||
| ultradumbBenchmark: ultradumbBenchmark, |
There was a problem hiding this comment.
this one is still toString()d within driver. can we update this one as well?
There was a problem hiding this comment.
I had him change it back because we use the raw function in the scripts
There was a problem hiding this comment.
Doesn't this break
?There was a problem hiding this comment.
Nah. It's still exposed the same way. It's just ALSO being exposed as a string.
patrick didnt like my idea of only exposing as string and evaling in the script/benchmark.js. :) haha
ghost
mentioned this pull request
Summary
Export all functions in lib page-functions directly as string
Minor coding style i would say
Prettier code due not cluttering implementation files with multiple toString() calls
Related Issues/PRs
#5870