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 source maps #1288

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Remove source maps #1288

merged 2 commits into from
Oct 7, 2022

Conversation

wirednkod
Copy link
Contributor

No description provided.

@MOZGIII
Copy link

MOZGIII commented Oct 7, 2022

Please see an explainer at #1285 (comment).
Proper source maps support is right there. We have already tested that solution and it works flawlessly.
Please don't make things worse, let's just merge that code.

@wirednkod
Copy link
Contributor Author

Please see an explainer at #1285 (comment). Proper source maps support is right there. We have already tested that solution and it works flawlessly. Please don't make things worse, let's just merge that code.

I think that removing the source makes things better rather than worse for the production code, and I think its preferable to keep the amount of files and size of it to minimum (removing it, reduces amount of files from 62 -> 48 and save some hundreds of KB).

The sample application you provided for supporting your comment is a CRA app. I have tested these changes in non-CRA apps/demos and were working without warnings.

There is an explanation here about CRA 5 and how the integrated in CRA webpack handles the sourcemaps. There is proposed a workaround that you can run in order to avoid warnings but in my opinion having source maps in production is kind-of-redundant and adds files and size to the final dist.

@wirednkod wirednkod added the automerge Instructs Mergify to queue this pull request for automatic merging label Oct 7, 2022
@mergify mergify bot merged commit 7700a88 into main Oct 7, 2022
@mergify mergify bot deleted the nik-source-map-fix branch October 7, 2022 14:02
@MOZGIII
Copy link

MOZGIII commented Oct 7, 2022

I think that removing the source makes things better rather than worse for the production code, and I think its preferable to keep the amount of files and size of it to minimum (removing it, reduces amount of files from 62 -> 48 and save some hundreds of KB).

You are wrong. The production code is shipped in bundles, and the way this works is libraries provide source maps, and whoever is using the library decides whether they want or don't want to use source maps.

The sample application you provided for supporting your comment is a CRA app. I have tested these changes in non-CRA apps/demos and were working without warnings.

I don't know what you're talking about, I didn't provide anything, this is the first time I've chimed in on this issue.

I'm also not just talking from a perspective on coming up with a hack, I just know how these things are supposed to work currently. In CRA or non-CRA, the source maps with properly filled source content will work.

There is an explanation here about CRA 5 and how the integrated in CRA webpack handles the sourcemaps. There is proposed a workaround that you can run in order to avoid warnings but in my opinion having source maps in production is kind-of-redundant and adds files and size to the final dist.

You know, CRA is not really intended for the experts, and sometimes (quite often really) the workarounds they provide are misused by people when a really good solution is available. Properly supporting source maps on the source level is a good solution. Removing them is a bad solution. How we can tell it's bad? We invented them out of nothing and made a whole system to make them work across the cross-cutting packages and build systems. This took a lot of work, but it has still happened. Nullifying this whole effort just cause you don't fully understand how it all works is pathetic. Especially when the working solution has been contributed by the community.

@wirednkod
Copy link
Contributor Author

wirednkod commented Oct 7, 2022

I don't know what you're talking about, I didn't provide anything, this is the first time I've chimed in on this issue.

Apologies about this. I was referring to an example app provided by the person who opened #1285. Got a bit confused there.

Nullifying this whole effort just cause you don't fully understand how it all works is pathetic. Especially when the working solution has been contributed by the community.

I have removed for now the source maps in order to cover a specific case concerning warnings, that was opened.
My intention, is to investigate further the case of source maps, based on the solution provided in #1285 in order to "understand how it all works".

I think I should mention at this point, that is really not polite and not called for, the accusations of "nullifying efforts" and "being pathetic". Especially when the intention (as said above) is to investigate the possible solutions (including the one proposed).

@MOZGIII
Copy link

MOZGIII commented Oct 7, 2022

I think I should mention at this point, that is really not polite and not called for, the accusations of "nullifying efforts" and "being pathetic". Especially when the intention (as said above) is to investigate the possible solutions (including the one proposed).

Wouldn't be this way if were more transparent about the intentions. I apologize too.

I have removed for now the source maps in order to cover a specific case concerning warnings, that was opened.

Yep, and I'm more confused about this PR, because #1285 would solve that one as well.

My intention, is to investigate further the case of source maps, based on the solution provided in #1285 in order to "understand how it all works".

Inf you plan to put in more work here - it's all good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instructs Mergify to queue this pull request for automatic merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants