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

Support secondary file extensions in resolve.input & resolve.output #… #1909

Closed
wants to merge 1 commit into from

Conversation

glromeo
Copy link

@glromeo glromeo commented Dec 11, 2020

…1340

Please let me know your thoughts about adding outPath to SnowpackBuiltFile
if this solution is acceptable I will tidy up the PR with tests & doc
At the moment I have a plugin which uses these changes which works both in DEV and BUILD

Changes

This tries to implement a solution for #1340

Testing

All current tests are green

TODO

Docs

NOT YET

WIP

@vercel
Copy link

vercel bot commented Dec 11, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/lva8r0cpw
✅ Preview: https://www.snowpack.dev

@glromeo glromeo marked this pull request as draft December 11, 2020 05:27
@glromeo glromeo changed the title WIP: Support secondary file extensions in resolve.input & resolve.output #… Support secondary file extensions in resolve.input & resolve.output #… Dec 11, 2020
// @ts-ignore: Deprecated
filePath: fileName,
filePath: filePath,
Copy link
Owner

Choose a reason for hiding this comment

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

this seems to be changing a lot of existing behavior to use new values. our entire plugin ecosystem relies on these values not changing

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I really appreciate the work here, but I think we're going to go in a different direction for this. It's a tricky feature to add, and I'd feel better about merging if we made fewer changes across the codebase to do it. Also, it's a good chance to get rid of extendedExt which never really delivered on dynamic length extension support.

I'm going to submit this in a separate PR. If you're testing this in your own project, I'd love for you to check out that PR and see if it works

@@ -161,7 +162,7 @@ class FileBuilder {
continue;
}
const outFilename = replaceExt(
path.basename(url.fileURLToPath(this.fileURL)),
path.basename(filePath),
Copy link
Owner

Choose a reason for hiding this comment

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

we're trying to use more file URLs instead of less, so would like to keep these as-is

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