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

@rspack/dev-middleware defalut export will cause node esm envirment crash. #2944

Closed
underfin opened this issue Apr 26, 2023 · 13 comments · Fixed by #3358
Closed

@rspack/dev-middleware defalut export will cause node esm envirment crash. #2944

underfin opened this issue Apr 26, 2023 · 13 comments · Fixed by #3358
Labels
breaking change team The issue/pr is created by the member of Rspack.

Comments

@underfin
Copy link
Collaborator

underfin commented Apr 26, 2023

System Info

Details

The middleware use default to export value, but when the import file is .mjs, the export value is object which has default value.

// a.mjs

import rspackDevMiddleware from '@rspack/dev-middleware'

rspackDevMiddleware()// hasn't default

Reproduce link

No response

Reproduce Steps

@underfin underfin added bug Something isn't working pending triage The issue/PR is currently untouched. labels Apr 26, 2023
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Apr 26, 2023
@underfin underfin added p1-important and removed bug Something isn't working pending triage The issue/PR is currently untouched. labels Apr 26, 2023
@underfin underfin added this to the Planned milestone Apr 26, 2023
@hardfist hardfist modified the milestones: Planned, 0.2.0 - Minor Apr 26, 2023
@hardfist
Copy link
Contributor

we should use module.exports = rspackDevMiddleware so it could be used in commonjs and esm the same way like webpack-hot-middleware

@stale stale bot added the stale label May 26, 2023
@hardfist
Copy link
Contributor

hardfist commented May 29, 2023

we should use module.exports = rspackDevMiddleware so it could be used in commonjs and esm the same way like webpack-hot-middleware

we can't export more types or variables if we use module.exports= rspackDevMiddleware, which seems not extensible in the future and now(https://github.com/web-infra-dev/rspack/blob/main/packages/rspack-dev-middleware/src/middleware.ts#L16), but if we choose use module.exports = { rspackDevMiddleware} which is not consistent with webpack-dev-middleware.
also related to #2851
@alexander-akait any suggestions?

@stale stale bot removed the stale label May 29, 2023
@alexander-akait
Copy link

Hm, can you use this?

function getRspackMemoryAssets(...args) {
  // Code
}

getRspackMemoryAssets.something = 1;

export default getRspackMemoryAssets;

@hardfist
Copy link
Contributor

Hm, can you use this?

function getRspackMemoryAssets(...args) {
  // Code
}

getRspackMemoryAssets.something = 1;

export default getRspackMemoryAssets;

this will work for variable but not for tyoe

@alexander-akait
Copy link

But for types you can use export type { MyType } (i.e. named export).

I don't understand the problem a bit, maybe I'm missing something?

@hardfist
Copy link
Contributor

hardfist commented May 30, 2023

currently we use export default rdm which will cause import rdm from '@rspack/dev-middleware failed in .mjs, because export default rdm is transpiled to

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = rdm;

but since nodejs doesn't recognize __esModule.

import rdm from '@rspack/dev-middleware' // rdm is { default: rdm} which is very weird

so we have two choices here

  • use module.exports
module.exports = rdm

then import rdm from '@rspack/dev-middleware' works fine, but we can't expose more type and variable any more

  • use named exports
export { rdm } 

which works perfectly on mjs but not consistent with webpack-dev-middleware

import { rdm } from '@rspack/dev-middleware' // rdm is rdm which is perfect

@alexander-akait
Copy link

@hardfist I see, but

then import rdm from '@rspack/dev-middleware' works fine, but we can't expose more type and variable any more

What do you want to export here and why export type {} does not satisfy for types and rdm.something = 1 for export variables and extra functions, it is a common pattern for CommonJS

@hardfist
Copy link
Contributor

hardfist commented May 30, 2023

What do you want to export here and why export type {} does not satisfy for types and rdm.something = 1 for export variables and extra functions, it is a common pattern for CommonJS

yes rdm.xxx = 1 works for variable, but how do we expose type which is not bound to variable, for example we have type called type RdmMiddlelWare = { name: 'xxx', apply(){} } how do we expose it?

@alexander-akait
Copy link

alexander-akait commented May 30, 2023

@hardfist What about this:

function rdm() {}

namespace rdm {
  export type RdmMiddlelWare = {
    name: "xxx";
  };
}

export = rdm;

@alexander-akait
Copy link

That is old CommonJs approach to do it 😄

@hardfist
Copy link
Contributor

That is old CommonJs approach to do it 😄

this is too hacky for me 😭, I'm gonna choose the simplest way 😁

@hardfist
Copy link
Contributor

@hardfist What about this:

function rdm() {}

namespace rdm {
  export type RdmMiddlelWare = {
    name: "xxx";
  };
}

export = rdm;

even it's kind of hacky, I buy into this way now 🤣

@alexander-akait
Copy link

It is not hacky, it is official way for CommonJS 😄 One day we will migrate on ESM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change team The issue/pr is created by the member of Rspack.
Projects
None yet
3 participants