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

add extractSourceMap option for modules #15523

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

vankop
Copy link
Member

@vankop vankop commented Mar 13, 2022

What kind of change does this PR introduce?
feature
closes #10265

Did you add tests for your changes?
not yet

Does this PR introduce a breaking change?
no

What needs to be documented once your changes are merged?
deprecate source-map-loader

@webpack-bot
Copy link
Contributor

webpack-bot commented Mar 13, 2022

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@vankop vankop force-pushed the feat/extract-source-map-plugin branch 4 times, most recently from a936c95 to 85efee7 Compare March 14, 2022 13:39
sokra
sokra previously requested changes Mar 15, 2022
lib/ExtractSourceMapPlugin.js Outdated Show resolved Hide resolved
lib/NormalModule.js Outdated Show resolved Hide resolved
lib/util/extractSourceMap.js Show resolved Hide resolved
@vankop vankop force-pushed the feat/extract-source-map-plugin branch from 059faf8 to 5931183 Compare April 14, 2022 13:25
@vankop vankop force-pushed the feat/extract-source-map-plugin branch 3 times, most recently from b06b19f to b1a7c37 Compare April 20, 2022 16:48
@vankop vankop changed the title add ExtractSourceMapPlugin add extractSourceMap option for modules Apr 21, 2022
@alexander-akait
Copy link
Member

@vankop Can you rebase too? Thank you

Copy link
Member

@TheLarkInn TheLarkInn left a comment

Choose a reason for hiding this comment

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

Besides these three changes, requested this looks good.

lib/util/extractSourceMap.js Outdated Show resolved Hide resolved
lib/util/extractSourceMap.js Outdated Show resolved Hide resolved
lib/util/extractSourceMap.js Outdated Show resolved Hide resolved
@TheLarkInn
Copy link
Member

Also rebase needed.

@snitin315 snitin315 force-pushed the feat/extract-source-map-plugin branch from 7444e32 to a602d8c Compare January 14, 2024 01:30
@webpack-bot
Copy link
Contributor

@vankop Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@TheLarkInn Please review the new changes.

@vankop vankop force-pushed the feat/extract-source-map-plugin branch from ab23406 to 059380c Compare April 11, 2024 07:25
new WebpackError(
`Failed to parse source map from '${sourceMappingURL}': ${parseError}`
)
);
Copy link
Member

Choose a reason for hiding this comment

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

Based on feedback during development of https://github.com/webpack-contrib/source-map-loader/, better to make an options for this, in real world a lot of packages have broken source maps and developer can't fix it in vendor libraries, so better generate a warning, because it really doesn't break a build, but allow to setup it to make an error, i.e.

type ExtractSourceMap = boolean | { errorSeverity: "none" | "warn" | "error" }

@@ -0,0 +1,3 @@
const a = 1;
// comment
//#sourceMappingURL=/a.map
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a test for remote source maps, i.e. http://github.com/webpack/webpack/test/configCases/source-map/my-source-map.js.map

);
}

if (map.sources && map.sources.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Does we support sections? like here https://github.com/webpack-contrib/source-map-loader/blob/master/src/index.js#L91 because I see them sometimes

return callback(null, input.replace(replacementString, ""), map);
}
);
};
Copy link
Member

Choose a reason for hiding this comment

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

Also let's implement https://github.com/webpack-contrib/source-map-loader/tree/master?tab=readme-ov-file#filtersourcemappingurl, sometimes some source maps are really broken and you can't do anything with them, so let's add:

type ExtractSourceMap = boolean | {  errorSeverity: "...", test: Rules, include: Rules, Exlcude: Rules }

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Code look good, let's do small improvements

@alexander-akait
Copy link
Member

Just for infromation we have https://github.com/webpack-contrib/source-map-loader and it works great a lot of time, so maybe some optimizations and inspiration you can take from there

@snitin315 snitin315 self-assigned this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: On Hold
Development

Successfully merging this pull request may close these issues.

move source-map-loader into core
7 participants