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

replace imports with new aliases #2934

Closed

Conversation

thesebas
Copy link

@thesebas thesebas commented Oct 5, 2022

Replace deprecated module-alias with built-in imports map. It is present since node v12 and will be soon available in bun (it works in bun-canary). My goal is to make this app runnable with bun and that's the first step to make it happen.

@KristjanESPERANTO
Copy link
Contributor

I like this, but it will break some third-party modules because they use the module aliases.

Here is an example: https://github.com/KristjanESPERANTO/MMM-PublicTransportHafas/blob/fafe3a369ae1e356d36d82030d56ffba499024bf/node_helper.js#L1

@rejas
Copy link
Collaborator

rejas commented Oct 5, 2022

I got to agree with @KristjanESPERANTO : As much as I like replacing 3rd-party-packages with build-in-fucntionality, breaking compatibility with modules is something that should be avoided at all costs.

I cant imagine that there is some backward compatible way to replace module-alias?

"logger": "js/logger.js",
"fetch": "js/fetch.js"
"imports": {
"#logger": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also work if we omit the #?

Copy link
Collaborator

@rejas rejas Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://nodejs.org/api/packages.html#packages_imports the imports must start with a # :-(

Copy link
Author

@thesebas thesebas Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it is done to avoid conflicts with real npm modules. Using fetch and logger aliases while real modules already exist seems risky.

@thesebas
Copy link
Author

thesebas commented Oct 5, 2022

I guess one could create real npm modules named node_helper (fetch and logger are already taken) and proxy it to internal one.

@rejas
Copy link
Collaborator

rejas commented Oct 5, 2022

I guess one could create real npm modules named node_helper (fetch and logger are already taken) and proxy it to internal one.

That sounds very hacky and hard to maintain...

But whats the problem with bun and module-alias? Maybe there is something we can fix otherwise....

@KristjanESPERANTO
Copy link
Contributor

What if we switch MagicMirror and its built-in modules but still leave module-alias in there for the 3rd party modules? This will not get @thesebas to his original goal, but we would have the core a bit more modern and no longer directly dependent on module-alias.

@thesebas
Copy link
Author

thesebas commented Oct 5, 2022

But whats the problem with bun and module-alias?

module-alias does some hacky tricks to achieve its goal and that's not possible in bun,

bash-5.2$ bun serveronly/index.js 
12 | var modulePaths = []
13 | var moduleAliases = {}
14 | var moduleAliasNames = []
15 | 
16 | var oldNodeModulePaths = Module._nodeModulePaths
17 | Module._nodeModulePaths = function (from) {
   ^
TypeError: Attempted to assign to readonly property.
      at /Users/thesebas/projects/MagicMirror/node_modules/module-alias/index.js:17:0

and additionally this: ilearnio/module-alias#113

@thesebas
Copy link
Author

thesebas commented Oct 5, 2022

What if we switch MagicMirror and its built-in modules but still leave module-alias in there for the 3rd party modules?

I'm really new to the project and wasn't aware that many 3rd party plugins are relying on this module, so, as you said, won't fix the problem, but ofc it would improve a bit the project.

@thesebas
Copy link
Author

thesebas commented Oct 5, 2022

After few hours on this my original goal (make it run on bun) is now even less achievable, seems that there are many hidden incompatibilities (like console-stamp trying to use console.Console constructor that is missing in bun's console implementation and few others), but trying to run it with a bit more restrictive bun exposed some ambiguities that could also be improved (like server = new (require("https").Server)(options, app); instead of server = require("https").Server(options, app);).

@KristjanESPERANTO
Copy link
Contributor

@rejas, what do you think about pursuing this further if @thesebas undoes the removal of module-alias in the js/app.js and package.json?

Old 3rd party modules should still be able to use it that way, but the core wouldn't depend on it anymore. And new modules could use the new approach.

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 16, 2022

note that we added the package.json alias because npm implemented some security checking for modules found in node_modules, but not in package-lock. it removes them

so it kept being deleted.

an alternative was to make a GitHub repo and then reference that as a dependency Mich decided on the alias. I think think was I 2.10 timeframe

rejas pushed a commit that referenced this pull request Mar 25, 2023
To fix issue caused by #3071

In order to still fulfill the new linter rule `import/order`, I replaced
the alias with the path.

I also see two other approaches, but I opted for the simplest one here
for now.

The other approaches:
1. Also create an alias for `app`.
2. Use new `imports` (like in PR #2934), but let `_moduleAliases`
untouched to stay compatible to 3rd party modules.

**Edit**: Oh, I thought of another option:
- Add `require("module-alias/register");`
@khassel
Copy link
Collaborator

khassel commented Sep 9, 2023

still appreciate the idea but the mentioned problems in the comments are not solved and no prgress since 11 months so closing this.

@khassel khassel closed this Sep 9, 2023
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

Successfully merging this pull request may close these issues.

5 participants