Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

CommonJS support #1

Closed
SaltyAom opened this issue May 30, 2023 · 3 comments
Closed

CommonJS support #1

SaltyAom opened this issue May 30, 2023 · 3 comments

Comments

@SaltyAom
Copy link

SaltyAom commented May 30, 2023

Elysia 0.5 add support for CommonJS by default, this should allow us to drop the additional step of using CLI to convert Elysia modules to CommonJS.

It would be nice if would you updated the package to support CommonJS as well.

@bogeychan
Copy link
Owner

Sounds good, I will give it a try.

But I think since you introduced code analysis, it won't work without a build step (before each run) because of bun transpile step, for example:

isFnUse

// Bun
let is = isFnUse(
  'set',
  `(context) => {
    if (handleOrigin(context.set, context.request), handleMethod(context.set), exposedHeaders.length)
      context.set.headers["Access-Control-Allow-Headers"] = typeof allowedHeaders === "string" ? allowedHeaders : allowedHeaders.join(", ");
    if (maxAge)
      context.set.headers["Access-Control-Max-Age"] = maxAge.toString();
    return new Response("", {
      status: 204
    });
  }`
);

console.log(is); // true

// Node
is = isFnUse(
  'set',
  `context => {
        handleOrigin(context.set, context.request);
        handleMethod(context.set);
        if (exposedHeaders.length) context.set.headers['Access-Control-Allow-Headers'] = typeof allowedHeaders === 'string' ? allowedHeaders : allowedHeaders.join(', ');
        if (maxAge) context.set.headers['Access-Control-Max-Age'] = maxAge.toString();
        return new Response('', {
          status: 204
        });
      }`
);

console.log(is); // false

bugs in Bun.build / bun build kinda blocks progress atm

@bogeychan
Copy link
Owner

I got it working without using Bun.build. I adjusted the following parts of isFnUse, in case you encounter the same problem in the future with bun.

// >> INSERT
fnLiteral = fnLiteral.trimStart();
const argument = fnLiteral.startsWith('(')
? // (context) => {}
fnLiteral.slice(fnLiteral.indexOf('(') + 1, fnLiteral.indexOf(')'))
: // context => {}
fnLiteral.slice(0, fnLiteral.indexOf('=') - 1);
// << INSERT

// >> INSERT
if (argument.includes(`{${keyword}`) || argument.includes(`,${keyword}`))
return true;
// << INSERT
if (argument.includes(`{ ${keyword}`) || argument.includes(`, ${keyword}`))
return true;

I'll look into CommonJS support tomorrow

bogeychan added a commit that referenced this issue May 31, 2023
@bogeychan
Copy link
Owner

CommonJS support should be added without build-step in @bogeychan/elysia-polyfills@0.5.1 and create-elysia@0.0.15:

yarn create elysia my-elysia-app --template node

If you want to support ESM hybrid with CJS in Elysia you can follow this blog post. I implemented the same approach.

Furthermore you have to use "moduleResolution": "nodenext", (i.e. specify all imports and exports with .js extension)

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

No branches or pull requests

2 participants