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

enable sourcemaps for bundles, include sourcemap for plugins in npm package #24303

Merged
merged 2 commits into from
Oct 19, 2017

Conversation

Delagen
Copy link
Contributor

@Delagen Delagen commented Oct 9, 2017

related #19063

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: why haven't you enabled sourcemaps for all JS files? Also, you should inline the source files to avoid issues like #19063.

@mdo @Johann-S: do you remember what was the reason for us not to include sourcemaps for JS files? It's like this from v3, so there must be some reasoning.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR As i see, dist/js/bootstrap.bundle.js.map contains all sources, and others also. I don't included recompiled files in commit!!!

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: even so, I'll verify after you address the other issue I mentioned above.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR Which issue? Include recompiled files in commit?

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: you haven't enabled sourcemaps for all of our dist JS files.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR enabled for minified files

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

We need includeSources and url for uglify-js. See https://github.com/mishoo/UglifyJS2#cli-source-map-options

Also, we need to do the same for js-minify-docs.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR updated. Seems includeSources have no effect and enabled by default, but enabled manually. Strange but in source code of UglifyJS it defaults to false.

As for me url comment is unneded if file has the same name. If file will be included under different name due user rename, use have to update comment also to make it works.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: url isn't a boolean; you need to specify the filename.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR strange behavior ))

As i see in compiled files
bootstrap.bundle.js

//# sourceMappingURL=bootstrap.bundle.js.map

bootstrap.bundle.min.js

//# sourceMappingURL=true

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

Because the bundle already has sourcemaps specified in rollup step.

@Johann-S: any ideas?

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR I understand. Updated
I don't use cli tools at all in my work )) so don't know all switches )

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

The sourcemaps still don't show the sources for me. I'll try to find why, otherwise if anyone has any ideas please comment here.

EDIT: Hmm, Chrome loads the source files fine. FF v57 beta doesn't here on Windows.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR I think it's due base&root source map settings. Many ../ in source paths.
But I see now all source maps )
image

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: that is with Chrome, right? Because with Chrome it works for me too but with FF it doesn't.

@XhmikosR XhmikosR dismissed their stale review October 9, 2017 12:15

changes made

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR Also in Firefox
image
I use Webpack for bundling. I'll test it when include simply in html

Chrome works
image

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

Ah, true. It's grouped under the (no domain) row.

All right, all seems good now.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR Yes. The problem is in base and root )

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@Delagen: do you have a fix for that?

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR I think rollup calculate paths relative to output, so I'll see how to tune this.

@Delagen
Copy link
Contributor Author

Delagen commented Oct 9, 2017

@XhmikosR I think if files required from node_modules, all be fine due long path to it, and it points to real files. If we change source mapping, we broke package debugging, so I think we must keep it unchanged.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 9, 2017

@mdo: are you OK with adding sourcemaps for JS files? I know we didn't include them in v3, not sure what the reasons were.

They don't hurt being present plus we already include sourcemaps for CSS files.

@XhmikosR XhmikosR requested a review from mdo October 9, 2017 13:17
@XhmikosR
Copy link
Member

@mdo: ping

@mdo
Copy link
Member

mdo commented Oct 19, 2017

I've no preference on including or not—that's a better question for @Johann-S and @bardiharborow.

@Johann-S
Copy link
Member

Not against it because we already provide source map for our minified CSS

@XhmikosR XhmikosR merged commit dc5a096 into twbs:v4-dev Oct 19, 2017
@XhmikosR
Copy link
Member

@mdo: when you do npm run dist next time, don't forget to include the new files.

@mdo mdo mentioned this pull request Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants