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

Distribute package as source #213

Merged
merged 2 commits into from
Jan 13, 2021
Merged

Conversation

mroderick
Copy link
Member

@mroderick mroderick commented Jan 8, 2021

@khamiltonuk and I spent some time trying to replace rollup with esbuild only to discover that it drops comments. That would bundling with esbuild and providing .d.ts files mutually exclusive.

I realised that we don't actually need bundling at all in this project.


This library is not meant for writing end user applications or even for being used directly when writing tests. It is not meant to be loaded directly by browsers.

Consumers of this package are assumed to use their own bundlers, should they need to bundle code for use in browsers or workers.

Therefore, it is safe to distribute the package as source files and not bundle them up.

This allows us to remove the bundler and its transitive dependencies, which reduces the maintenance burden of managing the library.

Background

  • We'd like to ship the library with .d.ts files, to allow improved experience for TypeScript users
  • We would like to reduce maintenance burden of the library

Solution

  • Distribute the package as source files
  • Create .d.ts files from the source files
  • Remove the bundler

How to verify

  1. Check out this branch
  2. npm ci
  3. npm run build
  4. Observe that there are .d.ts files in the types/ folder
  5. npm pack
  6. Observe that the .d.ts files are included in the package
  7. npm link
  8. In a sinon checkout:
  9. npm link @sinonjs/samsam
  10. npm test
  11. Observe that tests still pass

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #213 (94db34d) into master (aa6815e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #213   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines          512       512           
=========================================
  Hits           512       512           
Flag Coverage Δ
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa6815e...94db34d. Read the comment docs.

@mroderick
Copy link
Member Author

We can take this approach to the whole family of Sinon libraries and for sinon and referee provide bundled versions as well.

Adopting this approach would make it easier to adopt JSDoc to create API docs and .d.ts files for packages.

@mroderick
Copy link
Member Author

@mshaaban0 would you mind taking this for a spin in a TypeScript project to see if the type hints are still working?

Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

This setup means that we have to run npm run build to have the source files in the right place when linking locally. I'm wondering if it wouldn't be better to keep everything in lib and ship the generated type definitions in a typings folder instead of merging everything in dist. This isn't a TypeScript project after all, we just want to ship types along with the library. WDYT?

@mroderick
Copy link
Member Author

This setup means that we have to run npm run build to have the source files in the right place when linking locally

Do you mean when using it with npm link?

@mantoni
Copy link
Member

mantoni commented Jan 9, 2021

Do you mean when using it with npm link?

Yes. And generally, I'm not sure the copy step is needed. Type definitions don't have to sit next to the implementation, AFAIK.

@mroderick
Copy link
Member Author

I'm not sure the copy step is needed. Type definitions don't have to sit next to the implementation, AFAIK.

If they don't, then we could distribute a types/ folder. That would make things easier.

@mshaaban0 what do you think? Do the types have to sit next to the source files?

@mroderick
Copy link
Member Author

--outFile FILE                Concatenate and emit output to single file.
--outDir DIRECTORY            Redirect output structure to the directory.

These options suggest that we could easily put the output somewhere else. I'll try updating the PR and @mshaaban0 can help verify that it's still working well.

Copy link
Contributor

@mshaaban0 mshaaban0 left a comment

Choose a reason for hiding this comment

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

Screenshot 2021-01-10 at 21 49 50

Works as expected 👍

This library is not meant for writing end user applications or even for
being used directly when writing tests. It is not meant to be loaded
directly by browsers.

Consumers of this package are assumed to use their own bundlers, should
they need to bundle code for use in browsers or workers.

Therefore, it is safe to distribute the package as source files and not
bundle them up.

This allows us to remove the bundler and its transitive dependencies,
which reduces the maintenance burden of managing the library.
@mroderick mroderick force-pushed the use-tsc-no-bundling branch from 3666ece to 94db34d Compare January 11, 2021 16:05
@mroderick
Copy link
Member Author

@hexeberlin and I have pushed new commits for this, using the outDir option, pointing it to a types/ folder. This means that we won't need to run build all time time when linking this package, but only for when npm pack is run.

@mshaaban0 would you mind doing the test again? ❤️

Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@mshaaban0 mshaaban0 left a comment

Choose a reason for hiding this comment

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

Works, thank you 👍

@mroderick mroderick merged commit b6d98ca into sinonjs:master Jan 13, 2021
@mroderick mroderick deleted the use-tsc-no-bundling branch January 13, 2021 16:22
@mroderick
Copy link
Member Author

This has been published to the npm registry as @sinonjs/samsam@5.3.1

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.

3 participants