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

Feature: Add Extension Option to Support Custom Suffixes (useful for ESM builds) #174

Conversation

eberling
Copy link

@eberling eberling commented Oct 8, 2021

This change adds the option to add suffixes, primarily with file extensions in mind, which is a use case I needed to support ESM builds. Barrels thus accepts a string passed with -x and appends it to all exports.

Copy link
Collaborator

@BitForger BitForger left a comment

Choose a reason for hiding this comment

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

Hi @eberling, thanks for the PR.

I have a couple questions that would be great if you could address before any decisions are made. To clarify, I'm clicking request changes but that doesn't necessarily mean you need to change it. Just some clarification first would be great.

My only other request so far would be to remove the yarn.lock file.

Thanks

src/index.ts Outdated Show resolved Hide resolved
src/builder.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@eberling eberling force-pushed the feature/eberling/add-custom-suffix-option branch 2 times, most recently from fee9c7c to a53b58f Compare October 12, 2021 11:59
@eberling
Copy link
Author

Hi everyone, thank you for your feedback.

Following Changes:

  • Error Handling: anything but a string is accepted (although yargs automatically parses everything as string, so this is just future proofing. Also, passing no arg to -x parses as an empty string if i tested it correctly).
  • Improved Usage explanation in README
  • Added Tests

In regards to dynamically determining file extensions: I think this is a great idea and could be useful for me in the future as well. I would suggest to provide this option of doing it first, as this already covers some use cases and does not have to provide a guarantee to be able to support all possible types of mappings (although at this point i can only think of .ts and .d.ts that require a mapping , for all other cases such as .mjs and .json you can just leave the original extension).

src/builder.ts Outdated Show resolved Hide resolved
@eberling eberling force-pushed the feature/eberling/add-custom-suffix-option branch from a53b58f to b02a3b6 Compare October 20, 2021 13:27
@eberling
Copy link
Author

Hey,
I made the changes. Barrels now dynamically checks the ending and appends the correct extension. the parameter is a boolean and is toggled off per default.

@eberling
Copy link
Author

eberling commented Oct 29, 2021 via email

Copy link
Collaborator

@BitForger BitForger left a comment

Choose a reason for hiding this comment

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

Why is there a submodule being added here? Please remove this.

@BitForger BitForger added the Feature Adds a new feature label Oct 29, 2021
@BitForger BitForger added this to the v2.4.0 milestone Oct 29, 2021
@bencoveney
Copy link
Owner

bencoveney commented Oct 29, 2021

I'll be able to have a decent review of all that has been discussed above during next week.

I haven't used ES modules much myself, and it looks like there is still some misunderstandings about the problem were trying to solve (and I will probably have these myself!), so a sample project demonstrating what's happening would still be really helpful, or I can try to reproduce this myself using the information above to get a better understanding.

For the root problem: are we trying to support:

  • modifing extensions on imports? or
  • ESM project configuration?

They might both have the same symptoms, but the solution might be different in each case.

@BitForger
Copy link
Collaborator

@bencoveney ESM projects. It writes the .js extension in the index.ts file so that it properly supports the ESM spec. Reading the way TS 4.5+ is going to work for ESM helps understand the why behind the solution. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html

@bencoveney
Copy link
Owner

bencoveney commented Oct 29, 2021 via email

@bencoveney
Copy link
Owner

Hi @eberling

I have created a sample that I believe represents an ESM solution here: https://github.com/bencoveney/barrelsby/tree/example-esm/examples/esm

I'm continuing to look at the documentation above but if you get chance, it would be great if you could confirm this represents the kind of problem you are seeing.

For reference:

  • npm run generate-barrels - to build the barrels, passes okay.
  • npm run build - to run the typescript compiler, passes okay.
  • npm run run - to run the javascript using node, fails with:
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/ben/projects/barrelsby/examples/esm/src/esm-export-default-1' imported from /Users/ben/projects/barrelsby/examples/esm/src/index.js

@hoodie
Copy link

hoodie commented Nov 1, 2021

let me chime in here for @eberling
Yes this is basically the root of the evil. Because typescript is weird and allows you to leave out the extensions you might end up generating barrels that are not valid javascript.

@eberling eberling force-pushed the feature/eberling/add-custom-suffix-option branch from b02a3b6 to 1577149 Compare November 1, 2021 13:36
@bencoveney
Copy link
Owner

I've written up what I think looks like a reasonable plan for supporting ES Modules on #178. Please have a look at let me know if there is any feedback. I figured an issue (rather than a PR) would be a better place to have that discussion.

Thanks again everyone who has contributed to this so far!

@github-actions
Copy link

No activity has been seen recently, marking as stale. If this is a mistake please reach out to a collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Adds a new feature no-pr-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants