plugins icon indicating copy to clipboard operation
plugins copied to clipboard

@rollup/plugin-multi-entry: Maintain file output names when output.preserveModules = true

Open 814k31 opened this issue 3 years ago • 9 comments

Rollup Plugin Name: @rollup/plugin-multi-entry

This PR contains: Probably somewhere in the grey area of a bugfix and a feature

  • [x] bugfix
  • [x] feature
  • [ ] refactor
  • [ ] documentation
  • [ ] other

Are tests included?

  • [x] yes (bugfixes and features will not be merged without tests)
  • [ ] no

Breaking Changes?

Its "kind of" a breaking change? Breaking in the fact that people who use this plugin with preserveModules=true will experience a change by upgrading, however the change will be probably an outcome that they would expect from this plugin when using preverveModules=true

  • [x] yes (breaking changes will not be merged unless absolutely necessary)
  • [ ] no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

When preserveModules is set on the output, its expected that the filenames and structure will not be altered. This library is always setting the files names to be multi-entry.js and if there are multiple it is multi-entry1.js and so on.

My desired out come is that I can use the multi-entry plugin to target all src files within my project to be built so when using the library I can easily do import { ExampleComponent } from 'example-library/nested-directory/exampleComponent';

when the src file structure is: example-library -> src -> nested-directory -> exampleComponent.js

814k31 avatar Aug 09 '22 14:08 814k31

~~Because I dont have pnp on my machine, I haven't actually been able to run the tests unfortunately... just fyi but that should at least describe my expectation~~

Edit: Tests have been run now

814k31 avatar Aug 09 '22 14:08 814k31

I am open to other suggestions on how to solve this, one being allowing config.entryFileName to be set to false or null to fallback to the options which would go into making this a non breaking change. However I wanted to try provide a solution that doesnt bloat configuration options

814k31 avatar Aug 09 '22 15:08 814k31

@shellscape not to be a pain but could I bug you to have a look at this It's actually stopping my enterprise from adopting rollup in general

I'm very flexible on the implementation here

Really I just need a method to make sure the plugin doesn't always use the default entry filename so that I can use rollups default behaviour with preserveModules and this plugin together

814k31 avatar Aug 20 '22 02:08 814k31

I generally don't have much stake in this plugin. Please check out the history and which maintainers have contributed to it in the past for their feedback.

shellscape avatar Aug 20 '22 02:08 shellscape

@shellscape will do thanks for replying so quickly

814k31 avatar Aug 20 '22 02:08 814k31

Hello @guybedford @NotWoods

sorry to drag you guys in here randomly Could I ask you for some reviews?

As stated before I'm pretty flexible on implementation here

814k31 avatar Aug 26 '22 03:08 814k31

Since the other two maintainers are quiet at the moment, I'll chime in to try and get this moving forward. I generally don't like to merge breaking changes unless there's a large, community-wide reason to do so. That said, I think you could add another option such as preserveOutputFilenames so that when true, the same behavior is applied without a breaking change. I would have no issue merging that for you.

shellscape avatar Sep 02 '22 18:09 shellscape

Thanks @shellscape I'll update the pr when I have time and tag you when its done

814k31 avatar Sep 08 '22 02:09 814k31

@shellscape all updated to use a new config option instead

I've also updated the pr title to hopefully match the required format, though tbh I'm not totally aware of what the required format is...

either way, look forward to hearing from you :)

814k31 avatar Sep 14 '22 04:09 814k31

The change looks good now. We're going to be blocked from merging until #1270 lands (sorry about that). One thing to do in the mean time is to update the README with the new option.

shellscape avatar Sep 23 '22 17:09 shellscape

Sorry about the delay myself, README is updated :)

814k31 avatar Sep 30 '22 00:09 814k31

Hey @shellscape

I noticed there were some pr conflicts so I've gone through and fixed them up is it possible to get this merged in ASAP? It's now become a priority for my work as we are going forward with adopting rollup

Thanks!

814k31 avatar Oct 14 '22 23:10 814k31

thanks!

shellscape avatar Oct 24 '22 17:10 shellscape