Skip to content

Support conditional export resolution with custom resolver#505

Merged
XmiliaH merged 5 commits intopatriksimek:masterfrom
klaviyo:fix-conditional-export-resolve
Feb 2, 2023
Merged

Support conditional export resolution with custom resolver#505
XmiliaH merged 5 commits intopatriksimek:masterfrom
klaviyo:fix-conditional-export-resolve

Conversation

@nick-klaviyo
Copy link
Copy Markdown
Contributor

Support resolution of conditional export packages with customer resolver. This change implements the solution described in #504

@XmiliaH
Copy link
Copy Markdown
Collaborator

XmiliaH commented Feb 2, 2023

Thank you for the PR. Could you check that npm test is succeeding. Seems like eslint wants some changes.
Furthermore, it would be nice to edit the resolve method in index.d.ts to also allow to return the object.

@nick-klaviyo
Copy link
Copy Markdown
Contributor Author

nick-klaviyo commented Feb 2, 2023

Thank you for the PR. Could you check that npm test is succeeding. Seems like eslint wants some changes. Furthermore, it would be nice to edit the resolve method in index.d.ts to also allow to return the object.

Thanks - I've added the missing comma that was failing eslint and updated index.d.ts - let me know if that additional return type is what you were thinking.

Also: is there additional configuration I need to do locally in order to run eslint? If I run the pretest npm script locally I get thousands of errors across all the project files. Something must be off with my settings 🤔 NVM. I had some additional files eslint was catching

@XmiliaH
Copy link
Copy Markdown
Collaborator

XmiliaH commented Feb 2, 2023

let me know if that additional return type is what you were thinking.

I was thinking of { path: string, module?: string } as you can optionally also return a new module name that should be used for lookup.

And maybe add /test/additional-modules/my-es-module/index.js to the .eslintignore to make eslint happy.

@XmiliaH XmiliaH merged commit fe3ab68 into patriksimek:master Feb 2, 2023
@XmiliaH
Copy link
Copy Markdown
Collaborator

XmiliaH commented Feb 2, 2023

Seems good to me now. Thank you.

@nick-klaviyo
Copy link
Copy Markdown
Contributor Author

Thank you!

@nick-klaviyo nick-klaviyo deleted the fix-conditional-export-resolve branch February 2, 2023 21:39
@nick-klaviyo
Copy link
Copy Markdown
Contributor Author

@XmiliaH Do you know when this will be released?

@XmiliaH
Copy link
Copy Markdown
Collaborator

XmiliaH commented Feb 3, 2023

I am busy over the weekend. Will try to do it on Monday.

@XmiliaH
Copy link
Copy Markdown
Collaborator

XmiliaH commented Feb 5, 2023

The new version is released

@nick-klaviyo
Copy link
Copy Markdown
Contributor Author

The new version is released

Thanks!

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.

2 participants