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

Javascript tweaks #3211

Merged
merged 4 commits into from
May 8, 2023
Merged

Javascript tweaks #3211

merged 4 commits into from
May 8, 2023

Conversation

i8-pi
Copy link
Contributor

@i8-pi i8-pi commented Apr 29, 2023

A few improvements in the Javascript that should not impact functionality

The first commit

  • Adds the html plugin to eslint config to enable linting Javascript in html script tags
  • Adds a command in package.json to allow running npm run lint from the command line, which checks both js and html files
  • Updates github ci to run the same command

The second commit swaps plain objects with JS Maps where the objects are used as arbitrary key-value stores. Plain objects come with some predefined keys

const obj = {};
typeof obj.toString === 'function' // true

which can be a problem if the keys come from user input, while Maps are specifically designed to be arbitrary key-value stores
The code changes are mostly mechanical

obj[key]
// becomes
map.get(key)

obj[key] = value
// becomes
map.set(key, value)

delete obj[key]
// becomes
map.delete(key)

for (const key in obj)
  if (obj.hasOwnProperty(key))
    body
// becomes
for (const key of map.keys())
  body

@lminiero
Copy link
Member

lminiero commented May 4, 2023

THanks! I'll let @atoppi comment on this, since most of the JS stuff I'm clueless about.

Copy link
Member

@atoppi atoppi left a comment

Choose a reason for hiding this comment

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

Hello @i8-pi and thanks for the effort!

Since we are migrating core objects to maps, I'd like to switch Janus.sessions too.
Could you please add it to the PR?

Also please address the notes I've left.

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
html/janus.js Outdated Show resolved Hide resolved
@i8-pi
Copy link
Contributor Author

i8-pi commented May 6, 2023

I will update Janus.sessions to use Maps as well. This will be externally visible though, unlike the other two changes, so maybe we should mentions that in the changelog

@atoppi
Copy link
Member

atoppi commented May 6, 2023

I will update Janus.sessions to use Maps as well. This will be externally visible though, unlike the other two changes, so maybe we should mentions that in the changelog

Good point.
That might break things for people using that specific object. Anyway mixing things up (e.g. using objects here, maps there etc.) for similar structs is not a good design imho.

Pinging @lminiero to help taking a decision, since he is the main maintainer/user/hacker of the library.

Side note: in case we decide to proceed with sessions Map, we should also update the types.

@i8-pi
Copy link
Contributor Author

i8-pi commented May 7, 2023

I've made the requested changes. The Janus.sessions change is there if you want it. The typescript files don't mention Janus.sessions and no changes should be required for that

@atoppi
Copy link
Member

atoppi commented May 8, 2023

Thanks, merging!

@atoppi atoppi closed this May 8, 2023
@atoppi atoppi reopened this May 8, 2023
@atoppi atoppi merged commit 29299da into meetecho:master May 8, 2023
@i8-pi i8-pi deleted the js-tweaks branch May 20, 2023 11:21
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.

3 participants