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

Don't include map files in distributions #285

Merged
merged 1 commit into from Oct 20, 2020

Conversation

xhochy
Copy link
Contributor

@xhochy xhochy commented Jul 3, 2019

This reduces the size of the sdist from 4.5M to 1.3M for me.

These files are quite useful during development but not needed by endusers.

@SylvainCorlay
Copy link
Member

Wow I did not know map files were so big!

@vidartf
Copy link
Contributor

vidartf commented Jul 5, 2019

Source maps tend to be on the big side. There is one argument for distributing them normally, which would to get clearer errors copy-pasted by users from their browser console, but I'm not sure how much that is worth :)

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Jul 5, 2019 via email

@xhochy
Copy link
Contributor Author

xhochy commented Jul 5, 2019

An alternative approach would also be to distribute them in a separate package: conda-forge/mlflow-feedstock#9

@maartenbreddels
Copy link
Member

Rather than deleting them, should we not produce them at all in "production mode"?

You may still accidentally release them if they are generated (and didn't clean up).

I think we still want to have source maps at this moment to have reasonble error messages.

@jtpio
Copy link
Member

jtpio commented Jan 27, 2020

There is one argument for distributing them normally, which would to get clearer errors copy-pasted by users from their browser console, but I'm not sure how much that is worth :)

This is generally useful, but since the voila frontend is rather minimal, it might not be worth shipping them?

@SylvainCorlay
Copy link
Member

@jtpio @maartenbreddels I am thinking of getting this in for 0.2.0. I like the reduction of the bundle size.

@vidartf
Copy link
Contributor

vidartf commented Jun 2, 2020

I like the reduction of the bundle size.

Just to make sure: You are talking about the sdist size right?

@maartenbreddels
Copy link
Member

Just to make sure: You are talking about the sdist size right?

sdist and wheel (and thus conda package) I guess, what else could it be?

@vidartf
Copy link
Contributor

vidartf commented Aug 20, 2020

Just to make sure: You are talking about the sdist size right?

sdist and wheel (and thus conda package) I guess, what else could it be?

The published npm package? It's been so long I've lost all context here

EID:T I guess I wasn't used to thinking of sdist/wheels as "bundles", but instead I got concerned that the maps had been included in the JS bundle, instead of served separately ?

@maartenbreddels
Copy link
Member

I'd say we merge this, agree @SylvainCorlay?. I prefer this solution to not generating the map files since it might be useful for local debugging, and if it gets accidentally gets generated, we don't accidentally distribute it.

@maartenbreddels
Copy link
Member

Do we want to distribute this in voila-sourcemap maybe? That we release for each version in tandem?

@maartenbreddels
Copy link
Member

Let do a release with this first, it's easy to revert or change.

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.

None yet

5 participants