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

Remove the bundled dist/ directory? #362

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Remove the bundled dist/ directory? #362

merged 1 commit into from
Nov 13, 2018

Conversation

loganfsmyth
Copy link
Contributor

I'm in favor of removing the dist/ folder at this point because it's not clear who the target users are, and bundling is much more common now that it was when dist was created in 2015.

source-map already

  • requires preprocessing for older browsers since it uses let and const
  • only works on browsers with WASM
  • must be configured with mappings.wasm's location for browsers

Those are all a complex enough that I find it hard to see the average user dropping source-map into their application without their own bundler. Having the extra files also increases npm package size with minimal benefit, and complicates maintenance if we ever want to add other dependencies since they'd all need to be bundled into dist too.

If we get requests for this after it is removed, I'm 100% for exploring options, but since npm is the primary distribution mechanism right now, it seems like most users will not be using from the registry anyway. Given that, we could also instead include standalone bundles attached to the Github releases or some other channel.

@coveralls
Copy link

coveralls commented Oct 10, 2018

Pull Request Test Coverage Report for Build 527

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.063%

Totals Coverage Status
Change from base Build 522: 0.0%
Covered Lines: 876
Relevant Lines: 972

💛 - Coveralls

@tromey
Copy link
Contributor

tromey commented Oct 16, 2018

I think this is reasonable, but TBH I don't know what the impact might be. Also I think this should wind up in a release with an "incompatible" version bump. Perhaps @fitzgen should weigh in here.

@tromey
Copy link
Contributor

tromey commented Oct 16, 2018

Also this probably should also patch CHANGELOG.md.

@loganfsmyth
Copy link
Contributor Author

Yeah, I think everything for the URL changes means we'll need a new major version anyway, so that's part of why I'm trying to also post other patches that seem worth doing at the same time.

@loganfsmyth
Copy link
Contributor Author

@tromey I can't tell from your comment, do you want me to add to the CHANGELOG as part of this PR, or as part of a later update when we're ready to release?

@loganfsmyth loganfsmyth merged commit 968e0e3 into master Nov 13, 2018
@loganfsmyth loganfsmyth deleted the drop-dist-build branch November 13, 2018 22:39
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