Skip to content

refactor(cli): add entry point for bin #53

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

Closed
wants to merge 2 commits into from

Conversation

ChristopherPHolder
Copy link

@ChristopherPHolder ChristopherPHolder commented Sep 18, 2023

This MR addresses the issue from MR #49

image

@BioPhoton
Copy link
Collaborator

@ChristopherPHolder thx for having a look at this!

Did you try to execute the build artefact?
npx dist/packages/cli collect

Copy link
Collaborator

@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

Would a combination of bundle false + the additional entry point work for us?

@ChristopherPHolder
Copy link
Author

Would a combination of bundle false + the additional entry point work for us?

It is worth testing it.

@matejchalk
Copy link
Collaborator

I had to change the bin.js path, but otherwise this seems to work just fine 🙂

image

@BioPhoton
Copy link
Collaborator

Here another 🧩 of the puzzle:
https://github.com/favware/esbuild-plugin-file-path-extensions

@ChristopherPHolder
Copy link
Author

So it seems it is really not straight forward. Just to getting running for the moment i have converted the bin to a .js file and passed as an asset.

I know this is not optimal and i am looking into the configuration for getting the bundling correct without the issue of import file extensions.

@ChristopherPHolder
Copy link
Author

Relevent: evanw/esbuild#622

@matejchalk
Copy link
Collaborator

I'm OK with the bin.js asset as a workaround 🤷‍♂️

Although as mentioned in #73, maybe we don't need two entry points for the cli anyway, index.ts could just execute instead of only exporting a function. I'm not sure we need to support importing from cli, users that want programmatic usage should be able to import everything they need from core (or models, utils, etc.).

@ChristopherPHolder
Copy link
Author

I'm OK with the bin.js asset as a workaround 🤷‍♂️

Although as mentioned in #73, maybe we don't need two entry points for the cli anyway, index.ts could just execute instead of only exporting a function. I'm not sure we need to support importing from cli, users that want programmatic usage should be able to import everything they need from core (or models, utils, etc.).

I am in full agreement. What are your thoughts on this? @BioPhoton

@ChristopherPHolder ChristopherPHolder marked this pull request as ready for review September 29, 2023 13:54
@BioPhoton
Copy link
Collaborator

I'm in for the core package!
lets for now have only the bin.ts file present and merge the discussed approach for the cli package.
I'll setup a core package later.

@BioPhoton
Copy link
Collaborator

closed as implemented in #66

@BioPhoton BioPhoton closed this Oct 10, 2023
@BioPhoton BioPhoton deleted the add-bin-entry-point branch November 20, 2023 23:56
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.

3 participants