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

Add automatic Alias HQ registration for Node #48

Merged
merged 3 commits into from
Apr 24, 2022

Conversation

diegoulloao
Copy link
Contributor

@diegoulloao diegoulloao commented Apr 13, 2022

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:

  • docs/integrations.md - instructions updated for Node
  • package.json - v5.3.2 -> v5.4.0

Added

  • register.js new

@diegoulloao
Copy link
Contributor Author

@davestewart what about this?

@davestewart
Copy link
Owner

davestewart commented Apr 19, 2022

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
Copy link

andidev commented Apr 19, 2022

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

@diegoulloao
Copy link
Contributor Author

diegoulloao commented Apr 19, 2022

@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
Copy link
Owner

davestewart commented Apr 19, 2022

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:

  • why should Node get special treatment?
  • why should a specific integration be put in the root (what if others wanted the same for their framework?)
  • just because two – Node-specific – packages do it, doesn't mean Alias HQ has to

Maybe there are workarounds:

  • place the file in plugins src/plugins/module-alias/register.js
  • create "register" files for all frameworks, i.e. src/register/<framework>.js (slightly shorter - is that the issue?)
  • use an install hook to copy the file from src/plugins/module-alias to <package root>

The other options are:

  • I put my foot down and say "no, sorry - just create your own script, that's how it's designed"
  • we start with the integration doc, and review later
  • I stop worrying

I just want to explore this before making a decision.

Waiting for your feedback :)

@donferi
Copy link

donferi commented Apr 19, 2022

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 davestewart changed the title Feat register script Add automatic Alias HQ registration for Node JS projects Apr 20, 2022
@davestewart
Copy link
Owner

davestewart commented Apr 20, 2022

@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?

@davestewart davestewart changed the title Add automatic Alias HQ registration for Node JS projects Add automatic Alias HQ registration for Node Apr 20, 2022
@diegoulloao
Copy link
Contributor Author

diegoulloao commented Apr 20, 2022

@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
Copy link
Owner

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
Copy link
Contributor Author

diegoulloao commented Apr 21, 2022

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

@diegoulloao
Copy link
Contributor Author

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

Copy link
Contributor Author

@diegoulloao diegoulloao left a comment

Choose a reason for hiding this comment

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

All looks fine! jest tests passing.

@davestewart
Copy link
Owner

davestewart commented Apr 21, 2022

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
Copy link
Contributor Author

diegoulloao commented Apr 21, 2022

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
Copy link
Owner

davestewart commented Apr 21, 2022

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
Copy link
Contributor Author

diegoulloao commented Apr 21, 2022

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
Copy link
Contributor Author

diegoulloao commented Apr 23, 2022

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

@davestewart davestewart merged commit cbeae70 into davestewart:master Apr 24, 2022
@diegoulloao
Copy link
Contributor Author

🚀

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.

4 participants