@feathersjs/express^5 has a reused Symbol() across pkgs, but not actually equal

jamesvillarrubia commented 1 year ago

Steps to reproduce

Build a small Express based app with v5.

Modify the package.json to leverage aliases for v5 express and v5 core.

"dependencies": {
  "f5": "npm:@feathersjs/feathers@^5.0.0-pre.38",
  "f5_exp": "npm:@feathersjs/express@^5.0.0-pre.38"
}

Modify your app imports to use the aliases:

const feathers = require('f5');
const express = require('f5_exp');

Send a request to your sample service.

See 500 error:

TypeError: Cannot read properties of undefined (reading 'express')
at /Users/neptune/Sites/personal/tester/node_modules/f5_exp/lib/rest.js:51:36
at /Users/neptune/Sites/personal/tester/node_modules/f5_exp/lib/rest.js:12:32
at Layer.handle [as handle_request] (/Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/layer.js:95:5)
at trim_prefix (/Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/index.js:328:13)
at /Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/index.js:286:9
at Function.process_params (/Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/index.js:346:12)
at next (/Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/index.js:280:10)
at /Users/neptune/Sites/personal/tester/node_modules/f5_exp/lib/authentication.js:16:20
at /Users/neptune/Sites/personal/tester/node_modules/f5_exp/lib/authentication.js:8:32
at Layer.handle [as handle_request] (/Users/neptune/Sites/personal/tester/node_modules/f5_exp/node_modules/express/lib/router/layer.js:95:5)

Possible Root Cause

The error is caused by an empty options object here: https://github.com/feathersjs/feathers/blob/4fbbfff2a3c625f8e6929e5a09e2cf7b739ffe11/packages/express/src/rest.ts#L65-L66 This is returned by the getServiceOptions function, which is pretty straightforward: https://github.com/feathersjs/feathers/blob/4fbbfff2a3c625f8e6929e5a09e2cf7b739ffe11/packages/feathers/src/service.ts#L38-L40

It does a property check on the service for a SYMBOL('@feathersjs/service').

https://github.com/feathersjs/feathers/blob/4fbbfff2a3c625f8e6929e5a09e2cf7b739ffe11/packages/feathers/src/service.ts#L5

When I inspect in debugger, the data is there, but the SYMBOL is different from what was set as a property and what is being checked.

I've tried legacy bundling, but that didn't work either. I suspect that the SYMBOL is being created in different contexts, and therefore doesn't reference the same object. Seems like there's a dependency injection/inversion here from the express lib that could resolve this, but I don't fully understand the benefit of the SYMBOL usage.

Any guidance?

How this was discovered.

I'm trying to setup automated testing across multiple Feathers libraries that currently work in 4 and need to be extended to 5. Creating multiple branches for testing felt like an ugly solution since it would require a lot of pipeline revisions and tie the semVer of the library to the Feathers versions. I explored npm overrides, npm aliases, and legacy-bundling=true to see if I could deduce the reason the SYMBOL was different in the express function.

Expected behavior

Dove Express should return a normal response even when aliasing the libraries.

Actual behavior

Dove Express throws an error related to unreferenced SYMBOL

System configuration

Tell us about the applicable parts of your setup.

Module versions @feathersjs/feathers@^5.0.0-pre.38 @feathersjs/express@^5.0.0-pre.38

NodeJS version:

16 & 18

Operating System:

OSX

Browser Version:

n/a

React Native Version:

n/a

Module Loader:

CommonJS

jamesvillarrubia commented 1 year ago

Here's a link to an example repo: https://github.com/jamesvillarrubia/feathers_express_v5_symbol_issue

jamesvillarrubia commented 1 year ago

Found a way around it:

const feathers = require('../../node_modules/f5_exp/node_modules/@feathersjs/feathers');
const express = require('f5_exp');

But this feels wrong. I guess the expectation that two different imports would leverage the same runtime scope and share Symbols() seems ripe for a downstream issue.

As an idea, would modification of the commons createSymbol to be globally scoped work better? https://github.com/feathersjs/feathers/blob/4fbbfff2a3c625f8e6929e5a09e2cf7b739ffe11/packages/commons/src/index.ts#L98-L100

And then modify it to the following to use the global Symbol registry.

return typeof Symbol !== 'undefined' ? Symbol.for(name) : name

Or modifying the function to use a closure, such that the Symbol is created in the context of the originating scope and not the scope of the service.ts

daffl commented 1 year ago

I didn't know that Symbol.for was a thing. That's probably exactly what is needed here - though it's still kind of strange that the module that both of those libraries are dependent on seem to get executed twice. I'd be fine with a PR that uses Symbol.for.

feathersjs / feathers