svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Published 20 hours ago • verified publisher icon sveltejs

[chore]: store regexp as variable instead of defining it inline

Open ivanhofer opened this issue 2 years ago • 10 comments

Before submitting the PR, please make sure you do the following

  • [ ] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • [x] Prefix your PR title with [feat], [fix], [chore], or [docs].
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with npm test and lint the project with npm run lint

I was looking through the code of the preprocessor feature and I saw that all regular expression are inlined in the code. Storing a regex in a variable instead can increase the performance by 5-20% for subsequent runs.

I tried to find good names for the regular expressions. The "downside" of re-locating the regular expressins is the small overhead to jump around when reading code. But with regular expressions in general it is not that easy to reason about when reading them. So a good variable name may also be easier to understand than the expression itself ^^.

There are also other places in the code where this can be applied. But I want to see if this type of PR get's accepted before I change more parts :)

ivanhofer avatar Jul 24 '22 07:07 ivanhofer

seems like a reasonable change to me. do you want to finish making it elsewhere like you mentioned?

Sure, I can do that

ivanhofer avatar Aug 01 '22 06:08 ivanhofer

I have added some more. But there are still missing some places. I'll replace them the following days

ivanhofer avatar Aug 08 '22 05:08 ivanhofer

@benmccann I'm done with my changes. Please take a look at the PR and the naming of the variables

ivanhofer avatar Aug 10 '22 18:08 ivanhofer

looks like this one will need a rebase

I don't know is it weird to have a variable like regex_starts_with_Export and maybe it should be a lowercase "e" instead?

benmccann avatar Sep 13 '22 19:09 benmccann

I don't know is it weird to have a variable like regex_starts_with_Export and maybe it should be a lowercase "e" instead?

The reason why I used Export and not export is because the regex looks for the exact term "Export". Since "export" itself is a term that is used for many use cases in the JavaScript ecosystem I wasn't sure if the variable name is self descriptive enough.

But I can change it if you want. How should I proceed?

ivanhofer avatar Sep 14 '22 05:09 ivanhofer

But I can change it if you want. How should I proceed?

I think better follow the code conventions where internal variables are snake case

tanhauhau avatar Sep 14 '22 06:09 tanhauhau

Since I rebased, the svelte-element-svg test is failing, but I can't figure out what's the issue. Can someone please help?

ivanhofer avatar Sep 15 '22 06:09 ivanhofer

i think it's failing on master too, let me take a look

tanhauhau avatar Sep 15 '22 06:09 tanhauhau

@tanhauhau I created PR. https://github.com/sveltejs/svelte/pull/7869

baseballyama avatar Sep 15 '22 07:09 baseballyama

I have rebased this branch

ivanhofer avatar Sep 18 '22 06:09 ivanhofer

I don't know is it weird to have a variable like regex_starts_with_Export and maybe it should be a lowercase "e" instead?

The reason why I used Export and not export is because the regex looks for the exact term "Export". Since "export" itself is a term that is used for many use cases in the JavaScript ecosystem I wasn't sure if the variable name is self descriptive enough.

But I can change it if you want. How should I proceed?

Could naming it regex_starts_with_capitalized_export be an idea?

MathiasWP avatar Oct 13 '22 09:10 MathiasWP

@MathiasWP i think the current name regex_starts_with_term_export works too

tanhauhau avatar Oct 13 '22 12:10 tanhauhau