matthiaaas / express-file-routing

Flexible file-based routing for Express (like Next.js + additional features)
https://www.npmjs.com/package/express-file-routing
MIT License
83 stars 13 forks source link

Breaking Change: Add ESM and CJS support #18

Closed jonluca closed 1 year ago

jonluca commented 1 year ago

Changing the code from require to await import has it work both in ESM and CJS codebases.

This is a breaking change due to the async nature of the router. All code would need to be migrated from router(...) to await router()

matthiaaas commented 1 year ago

hey @jonluca, sorry for the delay in reviewing your PR. Looks great so far except for the switch of router() to an async function. Is there a way of avoiding this breaking change? It doesn't quite match other Express library APIs, or at least I cannot think of any that forces you to await something in app.use(await ...).

jonluca commented 1 year ago

Unfortunately not as far as I know - we could export two functions, an import one and a require one, and current users wouldn't have any breaking changes. Unfortuantely import is always async, as far as I can tell

https://stackoverflow.com/questions/51069002/convert-import-to-synchronous

I left a comment saying we could make it .then'd instead of awaited, but Im not sure if that'll even work

tompahoward commented 1 year ago

Firstly, thanks @matthiaaas for this package and thanks @jonluca for the PR.

Is there anything we can do to move this forward? I need ESM support.

Maybe new async methods for EMS (instead of changing the current interface) is the safest way for now and get feedback from the community?

matthiaaas commented 1 year ago

hey @tompahoward, thank you for being serious about this feature. Still, I am quite unsure on how to approach the introduction of asynchronous route generation.

This PR's current implementation will change the syntax to the following less intuitive one:

// before
app.use(router())
// or
createRouter(app)

// after
app.use(await router())
// or
await createRouter(app)

Initially, I disliked this. Primarily, not because it's a breaking API change, but because for my experience I rarely see such awaited syntax for Express middlewares.

But I think, it's the only way to add clean ESM support without introducing other delicate side effects. So I'm going to review this again, merge it & release it as v3 asap.

Thanks to @jonluca & sorry for the delay.

dword-design commented 1 year ago

Just a quick note: I'm not actively using the package but I'm using a similar one and I'm loading the route files via jiti since it allows to also use TypeScript, Babel etc. That's needed when e.g. running in dev mode. It's also used by nuxt to load the config. Also, the change would be non-breaking since jiti is sync.

matthiaaas commented 1 year ago

ESM & CJS is support now available as a beta with npm install express-file-routing@beta. Feedback is appreciated so we can ship this highly requested feature asap.

Updated docs covering the breaking API change can be found in the v3 branch's README.

cc: @tompahoward @jonluca