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

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

↪️ 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
Copy link
Contributor

@lustoykov lustoykov left a comment

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.

@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.

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 Outdated Show resolved Hide resolved
packages/core/parcel-bundler/src/assets/CSSAsset.js Outdated Show resolved Hide resolved
@mischnic

This comment has been minimized.

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.

@mischnic
Copy link
Member Author

Ready for review 🚀

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.

@@ -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
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

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.

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

4 participants