[WIP] lib: fix dual hazard #37
Conversation
MylesBorins
force-pushed
the
fix-dual-hazard
branch
from
a319889
to
3003e3e
Compare
|
@Raynos PTAL this now covers all interfaces, and assigns the singletons to the global using symbols with the properties being unwritable and unenumerable |
| @@ -6,6 +6,7 @@ extends: 'eslint:recommended' | |||
| globals: | |||
| Atomics: readonly | |||
| SharedArrayBuffer: readonly | |||
| parser: babel-eslint | |||
There was a problem hiding this comment.
eslint :( it understands ES modules but not Symbol ... ?
There was a problem hiding this comment.
doesn't understand import.meta yet. There is an active issue to fix this
| } | ||
| } | ||
| }; | ||
| Object.defineProperty(global, util.clientSymbol, { |
There was a problem hiding this comment.
Consider putting a single symbol for the library on global and then putting Client on an object.
Aka
util.get('Client') and util.register('Client', Client)
The util module can have a single symbol and store an object with fields on it. This will have less symbols and also less boilerplate per source file.
| @@ -1,47 +1,61 @@ | |||
| import util from '../util/index.js'; | |||
|
|
|||
| class Argument { | |||
There was a problem hiding this comment.
The Argument class is not a singleton. This is a non-issue due to it not being exported, but this class declaration should probably be in the else statement to reduce confusion
| @@ -7,7 +7,9 @@ export default { | |||
| external: [ | |||
| 'dgram', | |||
| 'events', | |||
| 'module', | |||
There was a problem hiding this comment.
This should not be necessary as we do not rollup the tests.
There was a problem hiding this comment.
I had put it here when i was using module in the source, that got refactored out but i forgot to remove this, thanks.
| @@ -6,6 +6,7 @@ extends: 'eslint:recommended' | |||
| globals: | |||
There was a problem hiding this comment.
yup, just needs to be globbed in the script that runs it
|
This PR solves the problem and ensures we only have one class for the package instead of two. However, it would be frustrating to apply this pattern to every file. The more I think about this, the more I feel like publishing your npm package with two entry points with two pieces of duplicate code is just a bad idea. That being said, I have already stopped using Writing the source code in ESM but only publishing the CJS code to NPM seems like the cleanest thing to do for now ... |
|
publishing only CJS to npm would preclude named exports, as you can only export a default from CJS. At that point there is very little benefit to authoring in ESM. Also realizing that the pattern used in this repo only works assuming your don't introduce ESM dependencies 😅 |
This is a very nice conclusion. 💔 |
Refs: #36