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

Disable sourcemaps for production build #771

Merged
merged 9 commits into from
Sep 2, 2019
Merged

Disable sourcemaps for production build #771

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fd86031
Disable sourcemaps for production build
stefaniacardenas Aug 19, 2019
eb1dad0
Bumped versions of loaders, refactor npm sourcemaps options
stefaniacardenas Aug 20, 2019
24b399d
Destructure in argument declaration and put back default value
stefaniacardenas Aug 21, 2019
751be11
Added changelog
stefaniacardenas Aug 22, 2019
6ace9df
Merge branch 'development' into feature/fix-sourcemaps-paths-cx-3789
Aug 23, 2019
d9af48b
Merge branch 'development' into feature/fix-sourcemaps-paths-cx-3789
Sep 2, 2019
eb1005d
Merge branch 'development' into feature/fix-sourcemaps-paths-cx-3789
stefaniacardenas Sep 2, 2019
875e2b3
Fixed indentation
stefaniacardenas Sep 2, 2019
887628f
Run webpack with quiet flag to for debug purposes
stefaniacardenas Sep 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
"browserstack-local": "^1.3.7",
"chai": "^4.2.0",
"chalk": "^2.4.2",
"css-loader": "^2.1.0",
"css-loader": "^3.2.0",
"eslint": "^5.15.3",
"eslint-plugin-flowtype": "^3.4.2",
"eslint-plugin-react": "^7.12.4",
Expand All @@ -101,7 +101,7 @@
"selenium-webdriver": "^4.0.0-alpha.1",
"source-map-loader": "^0.1.5",
"speed-measure-webpack-plugin": "^1.3.0",
"style-loader": "^0.23.1",
"style-loader": "^1.0.0",
"terser-webpack-plugin": "^1.2.2",
"travis-weigh-in": "^1.0.2",
"webpack": "^4.30.0",
Expand Down
66 changes: 36 additions & 30 deletions webpack.config.babel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,20 @@ const baseRules = [
}
];

const baseStyleLoaders = (modules=true) => [
const baseStyleLoaders = (modules, withSourceMap) => [
//ref: https://github.com/unicorn-standard/pacomo The standard used for naming the CSS classes
//ref: https://github.com/webpack/loader-utils#interpolatename The parsing rules used by webpack
{
loader: 'css-loader',
options: {
sourceMap: true,
modules,
getLocalIdent: (context, localIdentName, localName) => {
const basePath = path.relative(`${__dirname}/src/components`, context.resourcePath)
const baseDirFormatted = path.dirname(basePath).replace('/','-')
return `onfido-sdk-ui-${baseDirFormatted}-${localName}`
}
sourceMap: withSourceMap,
modules: modules ? {
getLocalIdent: (context, localIdentName, localName) => {
const basePath = path.relative(`${__dirname}/src/components`, context.resourcePath)
const baseDirFormatted = path.dirname(basePath).replace('/','-')
return `onfido-sdk-ui-${baseDirFormatted}-${localName}`
}
} : modules
}
},
{
Expand All @@ -62,38 +63,43 @@ const baseStyleLoaders = (modules=true) => [
autoprefixer(),
url({ url: "inline" })
],
sourceMap: true
sourceMap: withSourceMap
}
},
{
loader: 'less-loader',
options: {
sourceMap: true
sourceMap: withSourceMap
}
}
];



const baseStyleRules = (disableExtractToFile = false) =>
[{
rule: 'exclude',
modules: true
},
{
rule: 'include',
modules: false
}].map(({rule, modules})=> ({
test: /\.(less|css)$/,
[rule]: [`${__dirname}/node_modules`],
use:
[
disableExtractToFile || !PRODUCTION_BUILD ?
'style-loader' :
MiniCssExtractPlugin.loader,
...baseStyleLoaders(modules)
]
}))
const baseStyleRules = (options={}) => {
const { disableExtractToFile, withSourceMap } = options
Copy link
Contributor

Choose a reason for hiding this comment

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

why not deconstruct in the arguments declaration?

return (
[{
rule: 'exclude',
modules: true
},
{
rule: 'include',
modules: false
}].map(({rule, modules})=> ({
test: /\.(less|css)$/,
[rule]: [`${__dirname}/node_modules`],
use:
[
disableExtractToFile || !PRODUCTION_BUILD ?
'style-loader' :
MiniCssExtractPlugin.loader,
...baseStyleLoaders(modules, withSourceMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will disable the sourcemap for both dist and npm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From reading webpack-contrib/css-loader#652 it seems they fixed, but according to your test it didn't correct?

Yes, I saw the source code with the fix but the problem was still there on upgrading. I couldn't figure out if I need to pass a parameter to the previous loader to make it work.

Also, found this webpack-contrib/less-loader#80 (comment)

Starting with less-loader 4, source maps contain absolute paths now.

what?? why?? 😨

Yes, I read that but from the most recent discussions it seems like they've changed their minds
webpack-contrib/css-loader#886 (comment)

Copy link
Contributor Author

@stefaniacardenas stefaniacardenas Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this will disable the sourcemap for both dist and npm

Oops, will have a look

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @rfreitas on looking at the full file baseStyleRules is used in both the configDist and configNpmLib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had a default value in place then I did some refactoring and I removed it by mistake

Copy link
Contributor

Choose a reason for hiding this comment

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

it makes sense to me now 👍

]
}))
)
}


const WOOPRA_DEV_DOMAIN = 'dev-onfido-js-sdk.com'
const WOOPRA_DOMAIN = 'onfido-js-sdk.com'
Expand Down Expand Up @@ -327,7 +333,7 @@ const configNpmLib = {
module: {
rules: [
...baseRules,
...baseStyleRules(true)
...baseStyleRules({disableExtractToFile: true, withSourceMap: false})
]
},
plugins: [
Expand Down