Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

[FEAT] make rollup sourcemap independent of env #772

Merged
merged 5 commits into from
Jul 29, 2019

Conversation

teoxoy
Copy link
Contributor

@teoxoy teoxoy commented Jun 27, 2019

Adds sourcemap arg to the output function.

This issue was brought up in #590 (comment)

After #771 gets merged this is a crucial option to have imo

@teoxoy teoxoy mentioned this pull request Jun 27, 2019
@teoxoy
Copy link
Contributor Author

teoxoy commented Jun 28, 2019

By default (i.e. no sourcemap option is passed in) it will generate inline sourcemaps for dev and no sourcemaps for build.

@Conduitry
Copy link
Member

Is there anything else that needs to happen for webpack do you think?

@Conduitry
Copy link
Member

Actually, looking at this more, this doesn't seem like the cleanest way to go about this. This adds an argument to the config.xxx.output() functions, which would have to be documented somewhere. I think the proper way to do this would be to just return the output config object with the default sourcemap value in it, and let people override it with, for example, an object spread in their own config.

@Conduitry
Copy link
Member

Pushed that change and now I'm wondering what the purpose of this PR is now that I've changed it. Is there really a benefit to using inline sourcemaps? As far as I can tell, all this PR, as amended, does is default to inline sourcemaps, and also (conditionally) enable sourcemaps on the service worker.

As I mentioned above, I don't think we want to add an argument to the .output() functions that just overrides the value of a single field, when this is already possible using regular old js syntax.

@teoxoy
Copy link
Contributor Author

teoxoy commented Jul 29, 2019

Yeah, I guess overriding the sourcemap via object spread is an alternative. This is how I currently do it alltho I thought it was a workaround.

I would still merge this tho, it adds sourcemaps for the service worker by default and also uses inline sourcemaps for development which should cover the majority of use cases. Maybe change the title of the PR because it deviated from the initial intent.

@Conduitry Conduitry merged commit c653eef into sveltejs:master Jul 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants