[distro request] publish to npm..

[warren-bank]

Hi. I just found your repo.. and this is great! It's precisely what I was looking for.

To make install easier (for myself).. I forked your repo with the intention to only make a few minor tweaks to 'package.json'.. and publish a snapshot to mirror your most recent release.

Turned out, the code required updates.. to account for broken paths due to assumptions that don't hold (with a global install).

I just wanted to share with you the following links.. in case you should choose to do something similar and publish an official release to npm:

• repo
• diff of changes to code

Thanks again for all of your hard work! 😃

[cdot]

Thanks, Warren, for looking at this. I'd like very much to publish an npm package for Xanado, though at the moment I use github packages rather than npm for my other packages (for no particularly good reason, it was just that the template github actions I used worked that way.)

My concern with your proposed changes is the effect on the standalone version, which uses relative paths to find node_modules (there is no server to provide a resolution mechanism.)

You probably hadn't noticed that there's a 3.1.0 branch, that has a major refactoring away from requirejs to using esm instead. There are several reasons for doing this, not least that I wanted to split off the storage and dictionary modules for use in other projects, and future-proof things a bit. 3.1.0 also has a number of new features and bugfixes, and I'm in the process of getting it to build a webpacked version of the client code.

The obvious question is, how applicable are your proposed changes to the 3.1.0 branch? I can see the .npmignore and package.json changes as desirable, but the npm resolution mechanism is new to me, and it's unclear what it's intended to do.

[warren-bank]

I'll be the first to admit that my changes are a total hack..

I haven't used requirejs in many many years.. and they've been happier years for it.
I've never used it on the server, and the whole thing felt a bit alien to me.
Never-the-less,

• requirejs was configured to resolve npm dependencies using paths that are relative to a base directory (ie: the root of the project)
• npm doesn't actually guarantee that this is where these dependencies will be installed
  • so anytime the path to a npm dependency is needed, I used node's require.resolve(module) to resolve the absolute path to the module.. but not actually load it
  • the absolute path is handed off to requirejs.. and loading the module is delegated

Caveat,

• at the risk of going into unnecessary detail, I'll briefly mention that socket.io/client-dist/socket.io presented a unique problem
• socket.io/package.json restricts which files/directory are allowed to be exported.. and client-dist isn't allowed
• hence.. a super hacky workaround, which I can walk you through, if interested

With regard to your concern: effect on the standalone version

• honestly, didn't give it any consideration at the time
• strictly speaking, all paths to npm dependencies now use an absolute URL path that begins: /node_modules/...
  • so.. if the standalone version is being served by a web server (rather than a loaded in the browser from a file: URI), then..
  • if the root of the project is the root of the web server, it should work

Caveat,

• having just written the previous caveat, it occurs to me that the workaround for socket.io would break the standalone version
• the absolute URL path is changed to: /node_modules/socket.io/[..]/client-dist/socket.io
• to summarize the workaround (so you understand the problem):
  • the module: socket.io/client-dist/socket.io can't resolve, because the client-dist directory isn't exported
  • the module: socket.io can resolve, to: /path/to/node_modules/socket.io/dist/index.js
  • if we parse the desired module path: socket.io/client-dist/socket.io into 2x parts:
    1. socket.io
    2. client-dist/socket.io
  • then:
    • abs_base = path.dirname(require.resolve(socket.io))
    • abs_path = path.resolve(abs_base, client-dist/socket.io)
  • however:
    • abs_base is still within the dist directory
    • we need to resolve client-dist/socket.io relative to the parent directory
  • this means changing the absolute URL path
    • any change would break the standalone version
    • the preferred change (based on the resolution strategy just described) would be: /node_modules/socket.io/../client-dist/socket.io
      • however, either expressjs or requirejs.. somewhere along the line.. this URL is normalized and what the middleware actually sees is: /node_modules/client-dist/socket.io
      • not good..
    • hacky solution: change the URL path to: /node_modules/socket.io/[..]/client-dist/socket.io
      • allow the middlware to cleanup this special token by replacing all occurances of "[..]/" with "../".. before performing path resolution

With regard to refactoring:

• doing away with requirejs and only using either CommonJS or ESM to load modules
• using Webpack to bundle the client-side.. which natively understands both module loaders

..then my thoughts are:

• thank goodness!
• my changes are completely unnecessary
• once done, you shouldn't have any problem with a global npm install

In any case, sounds like you have a road-map.. and you're fully on top of things.
Please don't be distracted by my changes, as they were only just a bandaid;
I'm glad to see that you're addressing the issue in a more foundational way.
Best of luck!
And once again, thanks for providing such a great project.

[cdot]

With regard to your concern: effect on the standalone version
* if the root of the project is the root of the web server, it should work

The specific use-case requires that the project is installed below the root, hence the relative paths. Note that my ESM rewrite uses relative paths heavily, though of course they are pre-resolved in the webpack version.

    * **any change** would break the standalone version

Standalone doesn't use sockets (all comms are entirely within the browser, so the socket methods are stubbed).

Please don't be distracted by my changes, as they were only just a bandaid

Very welcome for that. The Javascript ecosystem is so confused that finding the "right" way to address problems isn't always easy, every commentator seems to have their own approach. I originally used requirejs simply because I've used it for years without problems, and I'm lazy. Now I've shifted to ESM+webpack I am finding it much more comfortable, though I am far from convinced I am using webpack correctly.

If you would like to try the 3.1.0 branch, then please do - npm run build should webpack the browser code. Run up the server and open dist/client_games.html (or simply open dist/standalone_games.html for the standalone version, no server required). A couple of packaging-related things remain to be done before it can be released:

• I haven't worked out how to get it to build the docker yet - it refuses to install modules from github packages.
• test the github actions for the npm build (and maybe target it to npmjs.com instead of github packages).

[cdot]

I just published 3.1.0 to npm as @cdot/xanado. I'll be interested to hear how you get on with it!

[warren-bank]

Glad to hear it :)

I didn't install it yet, but I have a few quick observations that roadblock even bothering to test:

• package.json doesn't have a "bin"
  • example.. though you would probably prefer to name your binary "xanado", rather than "scrabble"
• server.js doesn't have a shebang for nodejs
  • example
  • which nodejs ignores
  • which npm requires for the "bin" script to be recognized as requiring nodejs runtime

Those two things are the basic plumbing for publishing an executable to npm.
Now, whether or not paths can be properly resolved will need to wait until testing can be performed.

edit:
That last sentence isn't entirely true;
I could try to run the server manually.. and see if it starts or throws.
I'll go ahead and do that.. no need to publish an additional minor version to npm for that.
Be back shortly with results..

results:

npm ERR! code ENOENT
npm ERR! syscall spawn git
npm ERR! path git
npm ERR! errno -4058
npm ERR! enoent An unknown git error occurred
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

..do you have some kind of post-install script? ..which requires git

another thought..
which wouldn't explain why npm install was trying to spawn git..
is that package.json also doesn't have a "main".
I'd presume that in the absence of a "bin", npm is trying to install the package as a module..
which would also fail.. since the default main import file index.js doesn't exist.

update:
just noticed.. this

"dependencies": {
  "@rwap/jquery-ui-touch-punch": "github:RWAP/jquery-ui-touch-punch",
},

..why is an npm dependency being resolved from github, rather than the npm repository?

• github commits
• npm versions
• the current version in npm (1.0.11) includes all of the most recent commits to github
  • no reason to not use it

[warren-bank]

update:

• I added a new branch to my repo: fork/npm-publish/3.1.1
  • forks from commit: 008771b.. which is currently at the HEAD of your master branch
  • adds the few changes suggested above
  • pushed to npm: @warren-bank/xanado
    • my previous branch fork/npm-publish/2.1.3 is published to npm at: @warren-bank/scrabble
    • I didn't want to update the same npm repo
      • the previous one is working.. and I suspect this one will be badly broken
      • this new npm repo is temporary.. for testing and discussion; I'll unpublish it later..
  • status:
    • npm install --global @warren-bank/xanado succeeds
    • xanado fails
      • Error [ERR_MODULE_NOT_FOUND]
      • path to module is incorrect

[warren-bank]

update:

• I've made a few commits to my branch: fork/npm-publish/3.1.1
  • added a few tags.. each corresponding to an npm publish at: @warren-bank/xanado
  • performing some trial and error.. making some quick adjustments/iterations
• tag fork/npm-publish/v3.1.3
  • corresponds to the npm publish: @warren-bank/xanado@3.1.3
  • works great, for both:
    • multiplayer
    • standalone

[warren-bank]

soon...:

I'm going to do some housekeeping:

• remove all tags: fork/npm-publish/v3.1.*
• add a new branch: PR/3.1.1
  • squash the commits in the branch: fork/npm-publish/3.1.1
  • revert the npm scope from my own back to yours.. so it's clean to merge
• delete and recreate the branch: fork/npm-publish/3.1.1
  • squash the previous commit history
  • change the npm repo from @warren-bank/xanado to @warren-bank/scrabble
  • change the name of binary from xanado to scrabble
  • add tag: fork/npm-publish/v3.1.1
  • push update to my npm repo.. which is now simply a (nearly identical) mirror of your official repo
    • from 2.1.4 to 3.1.1
• unpublish npm repo: @warren-bank/xanado

update:

• all of the above housekeeping is done
• the branch PR/3.1.1 is associated with pull request fix: global npm install #84

[cdot]

I've merged the PR, though there's a problem with socket.io in the build (#85)

Two things caught my attention:

• README.md.md in the "files" in package.json. Why the extra .md?
• You took "https" out of the dependencies - which is corrent - but should it go to optionalDependencies? I've never been sure of the value of that, so happy to take advice.

Thank you especially for your work on the imports, I'm new to ESM and still learning!

[warren-bank]

• README.md.md in the "files" in package.json. Why the extra .md?
• You took "https" out of the dependencies - which is correct - but should it go to optionalDependencies? I've never been sure of the value of that, so happy to take advice. I guess if someone is capable of HTTPS then the dependency is moot.

• darnit.. you're right: README.md.md was a copy/paste error

  • I changed *.md.. and it was intended to be README.md
  • which was an unnecessary change, but intended to be more explicit.. as there is only a single .md file in the root project directory

• https is a module that's provided by nodejs

  • to include it as a dependency.. would be to import a 3rd party module
  • judging by its usage in the project.. I believe that you want node's

other thoughts/comments..

• this line:
  import * as SI from "socket.io/client-dist/socket.io.js";
  • might be problematic
    • socket.io doesn't export that directory
  • does this module only run client-side.. from within the webpack bundle?
    ..or does it also run on the server, as well?

[cdot]

socket.io comes in two flavours, server side and client side. The one that's failing is the client side (sensibly enough, since we don't webpack the server side ;-) )

There are 3 .md files in the root directory, README, DEVELOPING and CONFIGURATION. I think all three are needed in the npm module.

[warren-bank]

regarding the file ./src/client/ClientUIMixin.js..

I'm fairly certain that it isn't used by the server, since my server isn't crashing.. but I just want to be absolutely certain.. is it only ever used client-side.. from within the webpack bundle?

if so, then it's safe to revert this one import.. and use a relative path.. to workaround its overly restrictive package.json config.

---

regarding including markdown in the npm bundle..

you're right, I'm wrong.. there are 3x .md files.
personally, I'd only include the README;
the others just add unneccessary bloat to the npm bundle.

rule of thumb, anything that isn't needed to run the code (ie: module/app) should be excluded..
except README and LICENSE (which your repo doesn't include).

[warren-bank]

I'll update my PR branch..

[cdot]

Anything named client, browser or standalone is specific to the browser. server is specific to the server. Everything else might be used anywhere.

The README isn't written as the front page for an npm module, it's the front page for the github repository. It might be better to construct a specific readme for the npm, and point to github pages for the full doc. The license is MIT so no problem including some boilerplate.

[warren-bank]

I updated my PR branch.. with 2x additional commits.

npm run build will (once again) successfully build webpack bundles.
The dist/ directory is freshly rebuilt.
Running the server, everything looks fine on the client side.

[warren-bank]

I also:

• applied these changes to my branch: fork/npm-publish/3.1.1
• added the tag: fork/npm-publish/v3.1.2
• published to npm: @warren-bank/scrabble@3.1.2
  • in case you want to give it a test drive

One thing that's interesting.. which I haven't dug into yet:

• when I ran npm run build to refresh the content of the dist/ directory
• it produced an additional file:
  • dist/client/src_browser_Dialog_js-src_client_PasswordMixin_js.ClientGamesUI.js
• have you encountered this before?
  • I didn't make any changes to the webpack configs

[warren-bank]

PS: regarding the README.. this is what it looks like in npm

the only difference between our versions is that (in my fork) I tweaked the install instructions to reference my npm repo, rather than upstream.

[cdot]

webpack does a lot of weird stuff. I've added webpackChunk comments to try and tame it, but I'm not surprised to see it producing yet more module files.

The README is fine, it's really not worth faffing about with it. I have a few minor changes pending.

[cdot]

I get the following error now when trying to run the unpacked code (html/standalone_games.html)

Uncaught TypeError: The specifier “jquery/dist/jquery.js” was a bare specifier, but was not remapped to anything. Relative module specifiers must start with “./”, “../” or “/”.

[warren-bank]

isn't that to be expected?

• if you were to start the server (from the project directory)

    npm run server
  

• and were to load the URL into a browser:
  /html/standalone_games.html, then:
  • it's going to be running javascript that hasn't been bundled by Webpack
  • this includes imports that need to be resolved from npm dependencies (ie: node_modules)
• whereas, if you were to load the URL into a browser:
  /dist/standalone_games.html, then:
  • it's going to be running javascript from a Webpack bundle that includes all npm dependencies

I'd make the analogy that it's like trying to run typescript (or coffeescript) directly in node,
rather than running the javascript code that's produced by its transpiler; it doesn't work.

The purpose of Webpack is to prepare the (javascript) code to run in a web browser environment;
similarly, the purpose of the typescript transpiler is to prepare the (typescript) code
to run in a javascript runtime environment.

[warren-bank]

fwiw, I asked socket.io if they'd be willing to export the client-dist directory for use with Webpack..

I can't imagine that your project is unique in wanting to include this code in a client-side bundle.

[cdot]

Indeed. However one of my goals was a rapid development environment, where no transpilation step was required to run the code, and that's now broken. I would still prefer that, and have the transpiler map the paths, if it's possible to do it that way. I've grown used to the advantages. (I'm aware that an importmap would probably solve the problem).

[warren-bank]

if any javascript files are common to both the server and the client, then using node package resolution (which is also used by Webpack) is required.. and it simply won't work in the browser without pre-processing the client-side javascript into a bundle with Webpack.

if, however, all of the javascript that runs client-side is completely independent from the code that runs on the server.. then:

• you could revert all of the ESM import statements from node package resolution to using (brittle) hard-coded relative paths
• though I still would highly recommend against it

[warren-bank]

a better option might be to include webpack-dev-server into your workflow..
its features include hot reload, which allows for quick iteration.

links:
https://github.com/webpack/webpack-dev-server
https://webpack.js.org/configuration/dev-server/
https://webpack.js.org/guides/development/
https://codegodzilla.medium.com/how-to-hot-reload-with-webpack-ddf3a17fbd5e#97a0

If you want, I could add this to the repo.

[cdot]

I'm not going to revert the imports, I prefer them the way you have done it. I'm also not keen on bringing yet another tool into the dev workflow, it's already too baroque IMHO. At least not until we have explored the importmap option (I had it working a while ago, but I was doing it the wrong way round - imposing an importmap on the packed code, rather than removing an importmap required to run the code direct from the repository)

[cdot]

importmap did the trick.

[warren-bank]

you might want to take a peek at the commits that I've made to my dev branch:
https://github.com/warren-bank/fork-node-multiplayer-scrabble-server/commits/fork/npm-publish/3.1.1

it took a little trial and error, but..

• for production: (Webpack + Babel + Terser) is working great
• for development: (Webpack dev server + Xanado) is working great
  • without even looking, I've already found a client-side bug that I'll fix.. after getting some sleep

[cdot]

Excellent, good to hear it. I think we're in a good place right now! Curious about the client side bug, but I'll await your analysis.

[warren-bank]

oh, nothing major.. just noticed that jQuery was using a bad css selector..
so I was going to grep the src/ directory for other instances of similar mistakes.

in particular: this is what caught my eye:
$(".pauseButton")

• the top-right button during a multi-player game doesn't do anything, because it has the id="pauseButton".. not class
• when I was playing around with the Webpack dev server.. I quickly looked at the code for something that I could change and immediately see it reflected as an update in the browser
  • changing it to $("#pauseButton") made it work
  • chaining .text('Howdy!') made it more friendly :)

I just woke up.. sipping a coffee.. gonna lounge a bit.. haven't looked any farther into it beyond those earlier observations.

[cdot]

Tsk. I switched several classes over to using id's a while ago, I guess my grepping was less than perfect.

Enjoy your coffee!

[warren-bank]

I like your idea of using an import map (for each of the 4x html entry points)..
but I think I'm going to implement it a little differently than you have:

1. use a Webpack plugin to generate an import map (for each of the 4x .js entry points)
2. add a 1x liner to each of the 4x html entry points.. to load the corresponding import map
   • support for external import maps isn't yet available
   • a clever workaround will do for now
3. update the Webpack configs for building "dist"
   • to remove the 1x liner from each of the 4x html entry points (for distribution)

Using a modern web browser that supports ESM, this should be adequate for most local development.
And if more pre-processing is needed, then the Webpack dev server is always only an npm script away.

[cdot]

I tried (1) first, but webpack gave me so much grief that I abandoned it. (2) is fine so long as it's sustainable without too much additional cruft in the build. build/webpack_config.js is too complex already, IMHO.

[warren-bank]

darnit..

• the Webpack plugin that I mentioned previously doesn't support Webpack 5.x
  • so I forked it and updated it.. PR pending
  • annoying detour, but back on track..
• wired this new plugin up in build/webpack_config.js:

    import ImportMapWebpackPlugin from "@warren-bank/webpack-import-map-plugin";
  
    function getImportMapWebpackPluginOptions(js) {
      return {
        filter: (file) => (file.name && file.name.length && (file.name[0] !== '.')),
        transformKeys: (filename) => {
          const chunks = filename.split('/')
          let key = chunks[0]
  
          if (key && key.length && (key[0] === '@') && (chunks.length >= 2))
            key += `/${chunks[1]}`
  
          return key
        },
        baseUrl: '../node_modules/',
        fileName: `${js}.importmap.js`,
        serialize: (manifest) => (
    `{
      const im = document.createElement('script');
      im.type = 'importmap';
      im.textContent = ${JSON.stringify(manifest, null, 4)};
      document.currentScript.after(im);
    }`
        )
      }
    }
  
    // add to the configs Object returned by `makeConfig(html, js)` function:
    configs.plugins.push(
      new ImportMapWebpackPlugin(getImportMapWebpackPluginOptions(js))
    )

• everything worked exactly as it was supposed to.. but the data extracted by the plugin into an import-map isn't what I was expecting.. or hoping for:

    // ex: dist/client/ClientGamesUI.js.importmap.js
    {
      const im = document.createElement('script');
      im.type = 'importmap';
      im.textContent = {
        "imports": {
            "app.js": "../node_modules/client/ClientGamesUI.js",
            "GameSetupDialog.js": "../node_modules/client/GameSetupDialog.ClientGamesUI.js",
            "ChangePasswordDialog.js": "../node_modules/client/ChangePasswordDialog.ClientGamesUI.js",
            "AddRobotDialog.js": "../node_modules/client/AddRobotDialog.ClientGamesUI.js",
            "InvitePlayersDialog.js": "../node_modules/client/InvitePlayersDialog.ClientGamesUI.js",
            "findBestPlay.js": "../node_modules/client/findBestPlay.ClientGamesUI.js",
            "SettingsDialog.js": "../node_modules/client/SettingsDialog.ClientGamesUI.js",
            "GameDialog.js": "../node_modules/client/GameDialog.ClientGamesUI.js",
            "LoginDialog.js": "../node_modules/client/LoginDialog.ClientGamesUI.js",
            "client": "../node_modules/client/ChangePasswordDialog.ClientGamesUI.js.map"
        }
      };
      document.currentScript.after(im);
    }

so.. my plan worked perfectly.. but it's completely useless!
..darnit!

[warren-bank]

this is what I settled on..
it works.

I ended up producing the import map manually..
because the automated way turned out to be a rabbit hole that led me nowhere (good).

[warren-bank]

regarding the frontend jQuery css selectors..
I grepped for all instances of using a jQuery css selector that starts with a class..
cross-referenced the html DOM..
and confirmed that the pause button was the only problem..
though there were 3x times it was used..
this commit shows where they occur

[warren-bank]

regarding a tool to automate the generation of import map(s)..
it's too late now to bother with..
but it's worth noting that this looks very promising.