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

luci-app-squid: remove variable redeclaration #7511

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

dannil
Copy link
Contributor

@dannil dannil commented Dec 27, 2024

Since 22d4830 the form variables m, s, and o was changed to be declared with let instead of var, but the o variable was redeclared in the app, resulting in a redeclaration error when it was changed to instead be declared with let.

See https://forum.openwrt.org/t/openwrt-24-10-0-rc4-fourth-release-candidate/219368/81?u=dannil

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile N/A
  • Tested on: (x86/64, 24.10-rc4, Firefox) ✅
  • ( Preferred ) Mention: @ the original code author for feedback @stokito and @dannil (myself)
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: (describe the changes proposed in this PR)

Before
Screenshot_11

After
Screenshot_12

Since 22d4830 the form variables m, s, and o was changed to be declared
with let instead of var, but the o variable was redeclared in the app,
resulting in a redeclaration error when it was changed to instead be
declared with let.

Signed-off-by: Daniel Nilsson <[email protected]>
@systemcrash
Copy link
Contributor

There's probably quite a few bugs which are seeing light for the first time since lots of additions went into master which is seeing its first release in a while. This is a good thing.

@systemcrash systemcrash merged commit 235ec08 into openwrt:master Dec 27, 2024
4 checks passed
@dannil
Copy link
Contributor Author

dannil commented Dec 27, 2024

There's probably quite a few bugs which are seeing light for the first time since lots of additions went into master which is seeing its first release in a while. This is a good thing.

As a secondary point, we may want to check other apps if they have the same problem, I haven't seen any other reports of this issue on the forums but it could simply be that the apps aren't used often.

@dannil dannil deleted the squid-redeclare branch December 27, 2024 18:49
@systemcrash
Copy link
Contributor

As a secondary point, we may want to check other apps if they have the same problem, I haven't seen any other reports of this issue on the forums but it could simply be that the apps aren't used often.

Also true. Probably the case for stuff in master :)

@dannil
Copy link
Contributor Author

dannil commented Dec 27, 2024

I checked every app which had a diff in 22d4830 and squid was the only instance of this problem, so we should be safe now.

@hnyman
Copy link
Contributor

hnyman commented Dec 27, 2024

probably quite a few bugs which are seeing light for the first time since lots of additions went into master which is seeing its first release in a while. This is a good thing

Well, actually the stable 24.10 branch is seeing the release, not master. The stable 24.10 should not get any more speculative changes in the rc4 phase... For example, this 22d4830 has been authored well after 24.10 branching. Even this kind of innocent looking change can cause unanticipated problems, like this case proves.

24.10 should now really be let to stabilise, and no new things should be backported to it before having matured for a while in master. Rebasing on master worked well for the first weeks after branching, before RC builds, but right now we should let 24.10 to get stabilised, and hope that no new mishaps from the already backported new stuff materializes.

@stokito
Copy link
Contributor

stokito commented Dec 27, 2024

Good catch and sorry that I missed it. For some reason I expected that the use strict; that enables the Strict mode should already protect from such problems, but it doesn't.
My IntelliJ IDE also didn't shown any problems. The only inspection that reported the problem is JavaScript and TypeScript | Data flow | Reuse of local variable (disabled by default) but it's not about re-declaration but re-assignment (which often may lead to a bug).

Years ago I used the ESLint and will check if it can catch such problems.

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.

4 participants