davestewart / alias-hq

The end-to-end solution for configuring, refactoring, maintaining and using path aliases
https://davestewart.co.uk/projects/open-source/alias-hq/
MIT License
333 stars 12 forks source link

Add automatic Alias HQ registration for Node #48

Closed diegoulloao closed 2 years ago

diegoulloao commented 2 years ago

feat: register script

From #46

Provides a script to easily register paths from any package script just like module-alias:

E.g.

{
  "scripts": {
    "dev": "ts-node -r alias-hq/register src/main.ts"
  },
}

Also is possible to use with nodemon.

Changelog:

Added

diegoulloao commented 2 years ago

@davestewart what about this?

davestewart commented 2 years ago

Hey, thanks for your PR.

It seems from the code you added, that this code could all be local to your project:

{
  "scripts": {
    "dev": "ts-node -r ./config/alias-hq.js src/main.ts"
  },
}
// config/alias-hq.js
require('alias-hq').get('module-alias')

If so, I think it should just be an update to the Implementation doc, rather than a package update.

Can you confirm?

andidev commented 2 years ago

@davestewart would be convenient to have it included in the npm package. Is there a reason not to include it?

diegoulloao commented 2 years ago

@davestewart works exactly as you describe.

Actually is the same thing that module-alias and tsconfig-paths do. Those modules offer a way to register paths inside any package script.

tsconfig-paths Check the code: https://github.com/dividab/tsconfig-paths/blob/master/register.js Check the usage: https://github.com/dividab/tsconfig-paths#with-node

module-alias Check the code: https://github.com/ilearnio/module-alias/blob/master/register.js Check the usage: node -r module-alias/register src/main.js

I think it's a must to have feature and in my opinion, it must be included in the package as @andidev says.

davestewart commented 2 years ago

Hi both, and thanks for your input and thoughts.

The problem is, for this PR you're pleading for a "special case" for Node (though, good to know about the require flag!).

Where all the other integrations (WebPack, Rollup, Jest, etc) are expected to write their own config, you're asking that Node have its integration bundled with Alias (in the root, as well!) so you can just require a file.

The purist / maintainer part of me is torn; Alias HQ is a general package, so:

Maybe there are workarounds:

The other options are:

I just want to explore this before making a decision.

Waiting for your feedback :)

donferi commented 2 years ago

why should Node get special treatment?

IMO because node is not a random bundler, library, it can have special treatment.

It doesn't have to live in the root, but what if you create a package export for it to mimic it? 🤔

Post install hooks are a bad practice since they don't play nice with different module resolutions such as the ones used by pnpm and yarn's pnp.

davestewart commented 2 years ago

@donferi - nice! I didn't know about the export property and I think it is the right way to go.

I've updated it to work that way.

@diegoulloao - do you want to test it all works at your end?

diegoulloao commented 2 years ago

@donferi - nice! I didn't know about the export property and I think it is the right way to go.

I've updated it to work that way.

@diegoulloao - do you want to test it all works at your end?

Me neither.

I agree this is the way to go. Great. Let me check!

davestewart commented 2 years ago

Note that – for some reason the automated coverage testing is failing.

I think it's something to do with the GITHUB_TOKEN and permissions on this PR.

I can always ignore the actions and merge regardless.

diegoulloao commented 2 years ago

@davestewart sorry the delay. I just tested in my local and it's working fine. We can proceed! great job 🚀

diegoulloao commented 2 years ago

I have only one question: Why init instead register? Just a tiny detail.

davestewart commented 2 years ago

I have only one question: Why init instead register? Just a tiny detail.

Good question, and one I to'ed and fro'ed on...

In the TS Node project register() is actually not about "registering paths" but registering services:

The word "register" is mentioned 25 times on the main page!

As Alias HQ has no concept of services, and this is actually just an implementation detail in Module Alias, it didn't really make sense to use the verb "register"; it's actually just running / executing / initialising / <choose your verb here!> Alias HQ so I went with something generic.

I had also considered:

alias-hq/main
alias-hq/start
alias-hq/node
alias-hq/get/module-alias
alias-hq/run/module-alias
alias-hq/init/<plugin name>

The thing is, I think only Node will run like this; the rest of the integrations / plugins must return their content, so the multiple exports, i.e. alias-hq/init/<plugin name> is really just (an evil!) pre-optimisation.

Sound reasonable?

diegoulloao commented 2 years ago

The thing is, I think only Node will run like this; the rest of the integrations / plugins must return their content, so the multiple exports, i.e. alias-hq/init/ is really just (an evil!) pre-optimisation.

That was what I thought.

In the TS Node project register() is actually not about "registering paths" but registering services

Very good point I didn't notice 👌🏻

Definitely init is the most accurate option of all you mentioned. So, what's next move? 👀

davestewart commented 2 years ago

I check with another project (locally) that the tweaks (package exports) don't break anything, then in lieu of getting these f-ing Actions to work, probably just force the merge then we high five each other and kick back with Negronis.

I have a Webinar I need to prepare for tomorrow though, so it will probably be Saturday when I get on this again :)

diegoulloao commented 2 years ago

Negronis, I need to try that one 🤔

Ok, sounds like a plan. Good luck with the Webinar speech 😎 We are in touch until Saturday then!

diegoulloao commented 2 years ago

@davestewart, so is all okay for you? Merge?

diegoulloao commented 2 years ago

🚀