Apply auto lint fixer #56
Conversation
ddimaria
requested review from
John-LittleBearLabs,
ckousik,
plauche and
GlenDC
|
|
||
| export class InvalidFingerprintError extends WebRTCTransportError { | ||
| constructor (fingerprint: string, source: string) { | ||
| super(`Invalid fingerprint "${fingerprint}" within ${source}`) |
There was a problem hiding this comment.
For security reasons one does usually not add such details to an error, even though it might be UX. Dunno. Up to you.
There was a problem hiding this comment.
@TrophyNinjaShrub @ckousik thoughts?
There was a problem hiding this comment.
Can the fingerprint or source reveal any identifying information about the users or system?
There was a problem hiding this comment.
I have no objection to removing the fingerprint from the message. If this happens it would require a pretty in-depth investigation anyhow, and the debugger would have access to their own fingerprints.
@ddimaria - I didn't see your mention of me because notifications to my personal Github account go to spam.
There was a problem hiding this comment.
The fingerprint is public in the multiaddr and cannot reveal any information not available to an attacker.
There was a problem hiding this comment.
Overall the changes look good, but I have my concerns about how it will affect importing this package. I pulled down this branch locally, built it, and imported the local folder into our demo project. I saw breakage related to removing the .js file extensions in internal imports. This may not be an issue when we cut an actual release and import that, but I thought it was still worth raising.
|
|
||
| export class InvalidFingerprintError extends WebRTCTransportError { | ||
| constructor (fingerprint: string, source: string) { | ||
| super(`Invalid fingerprint "${fingerprint}" within ${source}`) |
There was a problem hiding this comment.
Can the fingerprint or source reveal any identifying information about the users or system?
src/maconn.ts
Outdated
| import { logger } from '@libp2p/logger' | ||
| import { Multiaddr } from '@multiformats/multiaddr' | ||
| import { Source, Sink } from 'it-stream-types' | ||
| import { nopSink, nopSource } from './util' |
There was a problem hiding this comment.
It's interesting that the linter wants to remove the .js file extensions from these imports. I had to add them in order to import this repo as a source dependency. Maybe that is all worked out once actual releases are made for importing?
There was a problem hiding this comment.
The linter did not request that, but I was able to get it working w/o the extensions, so went that way since it looks cleaner. Do I need to add them back (note: only some imports had the extensions).
There was a problem hiding this comment.
I'll take a look at the dist creation process, seems like this shouldn't be happening.
There was a problem hiding this comment.
Did you run aegir release?
There was a problem hiding this comment.
I found it was only required for imports from source files local to the package. It looks like the js-libp2p project retains the extensions for their imports from local package files, see their index.ts as an example.
There was a problem hiding this comment.
I ran npx aegir release on my local copy of this branch, and then attempted to build the demo source which imports the local branch copy. I still saw the same resolution error which even suggests adding the file extension:
Module not found: Error: Can't resolve './transport' in '/Users/ryanplauche/Development/LBL/Protocol/js-libp2p-webrtc/dist/src'
Did you mean 'transport.js'?
BREAKING CHANGE: The request './transport' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '*.mjs' file, or a '*.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.
There was a problem hiding this comment.
Interesting, I thought adding the extensions was an anti-pattern since it's done implicitly. I'll add them back.
There was a problem hiding this comment.
This is reverted. I also separated out the src imports from the package imports and did some OCD reordering.
There was a problem hiding this comment.
This seems to be the case in js-libp2p also.
| constructor(msg: string) { | ||
| super('There was a problem with the Multiaddr which was passed in: ' + msg); | ||
| this.name = 'WebRTC/InappropriateMultiaddrError'; | ||
| constructor (msg: string) { |
There was a problem hiding this comment.
I find this space very surprising, but if that's what the TS folks want 🤷
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I started to refactor, but then stopped as this should primarily be a lint-fix PR. We plan to fully document (jsdoc) all public functions, so just placeholders are in there for now. I'll fill those in within another PR.