Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
jamesvillarrubia opened this issue Feb 22, 2023 · 3 comments · Fixed by #3087
Closed

Comments

@jamesvillarrubia
Copy link
Contributor

Steps to reproduce

  1. Build a small Express based app with v5.
  2. 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"
  }
  1. Modify your app imports to use the aliases:
const feathers = require('f5');
const express = require('f5_exp');
  1. Send a request to your sample service.
  2. 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:

const options = getServiceOptions(lookup.service)
const middleware = options.express.composed

This is returned by the getServiceOptions function, which is pretty straightforward:
export function getServiceOptions(service: any): ServiceOptions {
return service[SERVICE]
}

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

export const SERVICE = createSymbol('@feathersjs/service')

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
Copy link
Contributor Author

@jamesvillarrubia
Copy link
Contributor Author

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?

export function createSymbol(name: string) {
return typeof Symbol !== 'undefined' ? Symbol(name) : name
}

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
Copy link
Member

daffl commented Feb 23, 2023

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants