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.

Sign up for GitHub

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

Jump to bottom

CSS/SASS/LESS sourcemaps #2489

Merged
merged 49 commits into from
Mar 5, 2019
Merged

CSS/SASS/LESS sourcemaps #2489

merged 49 commits into from
Mar 5, 2019

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Jan 1, 2019 •
edited
Loading

↪️ Pull Request

Proper (non inline) sourcemaps for PostCSS. Example

Closes #1824 Closes #903 Closes #2194 Closes #2692

💻 Examples

Anytime parcel minifies a css file

✔️ PR Todo

  • SASS (demo)
  • SASS tests
  • LESS (demo)
  • LESS tests
  • importing css files with source map comment
  • extract CSSAsset's and JSAsset's loadSourceMap function
  • extendSourceMap doesn't really work with new SourceMap().generateEmptyMap
  • rewrite sourcemap sources of imported file (and SASS?) to be relative to entry root
  • cache invalidation? (At which stage (Asset/Packager) should sourcemaps be disabled to make --no-sourcemaps work)
  • should it be app.js -> app.js.map or app.map

@mischnic mischnic added the 📝 WIP Work In Progress label Jan 1, 2019
lustoykov
lustoykov reviewed Jan 2, 2019
View reviewed changes
Copy link
Contributor

@lustoykov lustoykov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

really cool to see working sourcemaps for css!

@mischnic

This comment has been minimized.

Sign in to view
@mischnic mischnic removed the 📝 WIP Work In Progress label Jan 4, 2019
@mischnic mischnic changed the title CSS sourcemaps CSS/SASS/LESS sourcemaps Jan 4, 2019
@devongovett
Copy link
Member

Awesome work, really excited about this! 🎉

Will take a look soon.

DeMoorJasper
DeMoorJasper requested changes Jan 5, 2019
View reviewed changes
Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Looks good, left some comments about a couple lines of code that are identical to JS or obsolete and should probably be put into a seperate util or removed

packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
@mischnic

This comment has been minimized.

Sign in to view
DeMoorJasper
DeMoorJasper previously requested changes Jan 5, 2019
View reviewed changes
packages/core/parcel-bundler/src/SourceMap.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/SourceMap.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/SourceMap.js Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
@mischnic

This comment has been minimized.

Sign in to view
@mischnic mischnic mentioned this pull request Feb 27, 2019
Closed
@mischnic
Copy link
Member Author

Ready for review 🚀

devongovett
devongovett requested changes Mar 5, 2019
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

This looks amazing! Really nice work. 👏🔥🎉

Just one comment to fix and we can get this in.

packages/core/parcel-bundler/src/Asset.js Outdated
@@ -219,6 +219,19 @@ class Asset {
}

async postProcess(generated) {
let hasMap = false;
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this code to a separate method, or into the Pipeline class? If we add it here, you'll need to ensure to call super.postProcess from all assets that implement a postProcess method, as you've done for a couple already. That would be a potentially breaking change for plugins, so if we can move this outside that would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

DeMoorJasper
DeMoorJasper previously approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Except for the comment by Devon about the breaking change this looks good

devongovett
devongovett approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

@devongovett devongovett merged commit 8deb85d into master Mar 5, 2019
@devongovett devongovett deleted the css-sourcemaps branch March 5, 2019 18:11
wbinnssmith pushed a commit that referenced this pull request Apr 11, 2019
@mischnic @wbinnssmith
CSS/SASS/LESS sourcemaps (#2489)
592630d
@wbinnssmith wbinnssmith mentioned this pull request Apr 11, 2019
Merged
wbinnssmith pushed a commit that referenced this pull request Apr 12, 2019
@mischnic @wbinnssmith
CSS/SASS/LESS sourcemaps (#2489)
2856cf9
@polarathene polarathene mentioned this pull request Apr 20, 2019
Open
@quasicomputational quasicomputational mentioned this pull request Apr 21, 2019
Closed
@mischnic mischnic mentioned this pull request Apr 6, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lustoykov lustoykov lustoykov left review comments

@devongovett devongovett devongovett approved these changes

@DeMoorJasper DeMoorJasper DeMoorJasper left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mischnic @devongovett @DeMoorJasper @lustoykov