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

(fix): --name shouldn't change output filenames #669

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kotarella1110
Copy link
Contributor

@kotarella1110 kotarella1110 commented Apr 8, 2020

Fixed output file name after build to use the name of package.json instead of the passed name if the --name option is passed.

Currently, the output file name use the name passed with the --name option, as follows:

  1. Create a library using hyphen: npx tsdx create my-lib
  2. Build with --name option: yarn build --name MyLib --format esm,cjs,umd
  3. Check dist dir: tree dist/
dist/
├── index.d.ts
├── index.js
├── mylib.cjs.development.js
├── mylib.cjs.development.js.map
├── mylib.cjs.production.min.js
├── mylib.cjs.production.min.js.map
├── mylib.esm.js
├── mylib.esm.js.map
├── mylib.umd.development.js
├── mylib.umd.development.js.map
├── mylib.umd.production.min.js
└── mylib.umd.production.min.js.map

I think the output file name should use the name of package.json instead of the name passed with the --name option, as follows:

dist/
├── index.d.ts
├── index.js
├── my-lib.cjs.development.js
├── my-lib.cjs.development.js.map
├── my-lib.cjs.production.min.js
├── my-lib.cjs.production.min.js.map
├── my-lib.esm.js
├── my-lib.esm.js.map
├── my-lib.umd.development.js
├── my-lib.umd.development.js.map
├── my-lib.umd.production.min.js
└── my-lib.umd.production.min.js.map

In fact, in ReactDOM, it is react-dom.development.js, not reactdom.development.js.

@agilgur5 agilgur5 changed the title (fix): fix output file name after build (fix): --name shouldn't change output filenames Apr 8, 2020
@agilgur5 agilgur5 added version: minor Increment the minor version when merged kind: bug Something isn't working labels Apr 8, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Apr 8, 2020

This is somewhere in between a bug fix and a behavior change/feature, because the docs and the help dialog say it's only for UMD names, but apparently it's used for the output filename in the source code too. I didn't realize that either and have touched all of this code (though these pieces weren't written by me), good catch!

I'll have to look at the history and possibly microbundle history to see if this is an evolution of the --name use-case or if it's a bug that it's used this way in the code.

In any case, this is a breaking change as there might be folks who use this undocumented behavior.
There's another related request back in #208 (comment) / #219 around changing the output name to always just be dist/index.js instead of this safePackageName stuff which I agree with, for consistency's sake. Changing that would fix this too, but that change is extremely breaking and this one not so much.

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Instead of changing it to outputName everywhere, should leave name as appPackageJson.name and add umdName as opts.name.
That requires less changes and is more explicit too. Would need to change Rollup's output.name accordingly

src/index.ts Show resolved Hide resolved
@kotarella1110
Copy link
Contributor Author

@agilgur5 Do you have any update on this?

@agilgur5
Copy link
Collaborator

agilgur5 commented Apr 13, 2020

So I did some investigation and this indeed some likes an unintentional bug that --name changes the filename on top of the UMD name. The docs both here and in microbundle say it's just for UMD.
microbundle has a very similar bug that will cause, in its case, options.name to be set to the --name flag, but it doesn't affect microbundle nearly as much because its filenames are set to whatever you've specified in your package.json fields. That might be a good feature for us to have, which would make any filename changes a bit less breaking (though itself would similarly be breaking if a user did any renames on their own). It was previously proposed in #219 as well.

That being said, because this is unfortunately a breaking change, this won't be able to go out soon.
v0.14.0, the next minor (minors are breaking in v0.x), is already going to be a pretty big release with a handful of dependency-related potentially breaking changes, so this won't make it in there either 😕 (don't want to overload users with too many breaking changes at once)
Probably won't be till at least v0.15, so I wouldn't hold out on this getting merged in soon unfortunately.

In the meantime, if you need this, I'd recommend using patch-package or configuring tsdx.config.js to get this behavior, sorry about the unfortunate timing / breaking-ness.


Also in the meantime, I'll still make some review comments and I'll give credit to you with allcontributors for this bug report and code, esp since some code (the removals) was already used in #671 .

@agilgur5
Copy link
Collaborator

@allcontributors please add @kotarella1110 for bug, code

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @kotarella1110! 🎉

@agilgur5 agilgur5 added this to the v0.15.0 milestone Apr 13, 2020
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I still need to think a bit about this as it's a different change than I expected but might actually make more sense than what I was thinking. This is definitely better in any case, thanks for the iteration.

We should ideally add a regression test for this to ensure that setting --name doesn't change the actual filename. If you'd like to dive into the weeds of the test set-up you can do that yourself, but otherwise I'll do it myself eventually because we have to add a new test suite/file for passing options (test/e2e/tsdx-build-options.test.ts) and that's a lot more involved than just adding a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working version: minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants