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: add option to customize asset file names #41

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Oct 9, 2023

Resolves

mergeConfigs doesn't work for build.rollupOptions.output[].assetFileNames():

  • In the lib config it merges output arrays and results in both default and new assetFileNames, duplicating assets
  • In the app config it just replaces appConfig.ts values, requiring to override all cases, not only one specific, or duplicate this config defaults.

This PR adds new option assetFileNames to manually merge custom assetFileNames with the default. If custom one returns undefiend (falsy), then the default one is used.

Example

If a lib has styles and its entrypoint is main.ts, then styles are built to main.css and it is not possible to change.


@susnux If this options is fine, what do you think about adding the same for chunk file names to not more everything to one folder?

@ShGKme ShGKme added enhancement New feature or request 3. to review Ready to review labels Oct 9, 2023
@ShGKme ShGKme requested a review from susnux October 9, 2023 12:16
@ShGKme ShGKme self-assigned this Oct 9, 2023
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #41 (b1e4978) into main (63c6365) will increase coverage by 2.11%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   79.70%   81.81%   +2.11%     
==========================================
  Files           8        8              
  Lines         547      572      +25     
  Branches       32       46      +14     
==========================================
+ Hits          436      468      +32     
+ Misses         79       58      -21     
- Partials       32       46      +14     
Files Coverage Δ
lib/baseConfig.ts 87.09% <100.00%> (+0.79%) ⬆️
lib/appConfig.ts 80.00% <62.50%> (+1.05%) ⬆️
lib/libConfig.ts 90.72% <62.50%> (+1.91%) ⬆️

... and 1 file with indirect coverage changes

* Similar to `output.assetFileNames` in rollup config,
* but if returns undefined, then this config defaults is be used.
*
* @example Move CSS styles to `styles/style.css` instead of the default `css/[entrypoint-name].css`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not a good example as you will have to set cssCodeSplit to false. Otherwise the styles would override each other.

Copy link
Contributor Author

@ShGKme ShGKme Oct 12, 2023

Choose a reason for hiding this comment

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

The example was based on nextcloud/password-confirmation, where currently styles are renamed manually by adding && mv ./dist/main.css ./dist/style.css (only in a production build, dev build just doesn't work).

There is no cssCodeSplit and this custom function works.

But there is only one entrypoint

ShGKme and others added 2 commits October 18, 2023 13:23
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the feat/customize-assets-file-names branch from ff76547 to b1e4978 Compare October 18, 2023 11:23
@susnux susnux merged commit 67e1a38 into main Oct 18, 2023
10 of 11 checks passed
@susnux susnux deleted the feat/customize-assets-file-names branch October 18, 2023 11:37
@susnux susnux mentioned this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants