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

feat: derive proper js extension from package type #8382

Merged
merged 7 commits into from May 30, 2022

Conversation

patak-dev
Copy link
Member

Description

See #8348 (comment)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

: libOptions
? resolveLibFilename(libOptions, output.format || 'es', config.root)
? resolveLibFilename(libOptions, format, config.root)
: path.posix.join(options.assetsDir, `[name].[hash].js`),
chunkFileNames: libOptions
? `[name].[hash].js`
Copy link
Member Author

Choose a reason for hiding this comment

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

@sachinraja @antfu shouldn't chunks also have a derived mjs/cjs/js extension for chunks in lib mode (and also SSR?)

Copy link
Member

Choose a reason for hiding this comment

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

To me I think it's better to consist the extension for both entry and chunks

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they must to be compliant with node resolution

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, it should now work for SSR and chunks

@patak-dev patak-dev changed the title feat: derive proper ssr js extension from package type feat: derive proper js extension from package type May 29, 2022
@patak-dev patak-dev requested a review from antfu May 29, 2022 16:18
@patak-dev
Copy link
Member Author

We discussed with @dominikg that the logic in this PR and in #6827 does not cover the case where there is a package.json in a folder between root and the generated asset. @sachinraja were there discussions about this that I missed while you worked in #6827? We may need to check perf if we implement a check for every parent folder, but may be more robust.

I still think this isn't a blocker though. We should still merge this PR as it is an improvement over the current state, and we can discuss these changes (if needed) in a future PR.

@sachinraja
Copy link
Contributor

There were no discussions about it unfortunately. IMO it's not necessary but would be nice to have.

@patak-dev
Copy link
Member Author

Ok, let's merge this PR for now. Maybe we should wait to get a feature request with a real use case.

@patak-dev patak-dev merged commit 95cdd81 into main May 30, 2022
@patak-dev patak-dev deleted the feat/derive-filename-extension branch May 30, 2022 20:03
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.

None yet

3 participants