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

JENKINS-62473 Add source maps #4753

Merged
merged 1 commit into from
May 31, 2020
Merged

JENKINS-62473 Add source maps #4753

merged 1 commit into from
May 31, 2020

Conversation

95jonpet
Copy link
Contributor

@95jonpet 95jonpet commented May 27, 2020

See JENKINS-62473.

Adds source maps for built JS and CSS using reasonable settings from the available options. Production source maps are generated optimized for quality, whereas development source maps are of lower quality and faster to produce.

The solution uses the mode value passed to webpack from package.json:

"scripts": {
    "dev": "webpack --config webpack.config.js",
    "prod": "webpack --config webpack.config.js --mode=production"
},

Proposed changelog entries

  • Developer: Add source maps for CSS and JS.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@timja

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@timja timja requested review from fqueiruga and timja May 27, 2020 20:47
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label May 27, 2020
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'd guess we will need to test it with the CSS update effort

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

JavaScript seems to be working, but the less => css sourcemaps don't seem to be working, would you be able to take a look please?

@95jonpet
Copy link
Contributor Author

I will check it. It produces separate files in production, but should be inlined when developing. Perhaps there is no change that needs to be mapped? Right now it should just give line numbers in dev.

@timja
Copy link
Member

timja commented May 28, 2020

I will check it. It produces separate files in production, but should be inlined when developing. Perhaps there is no change that needs to be mapped? Right now it should just give line numbers in dev.

possibly I didn't check the production mode but its definitely giving the css files rather than the less files in chrome developer tools and if I check the css files I can't see any mention of a mapping file or inline mapping

@fqueiruga
Copy link
Contributor

fqueiruga commented May 28, 2020

I think I'm testing this all wrong. Maybe I'm missing something here. I'm not able to have this working.

@95jonpet
Copy link
Contributor Author

I will check this again and fix it.

@95jonpet
Copy link
Contributor Author

Source maps are generated in the .js-files för development, but seem to use absolute file paths:

eval("// extracted by mini-css-extract-plugin//# sourceURL=[module]\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiLi9zcmMvbWFpbi9qcy9hZGQtaXRlbS5sZXNzLmpzIiwic291cmNlcyI6WyJ3ZWJwYWNrOi8vLy4vc3JjL21haW4vanMvYWRkLWl0ZW0ubGVzcz82YmNjIl0sInNvdXJjZXNDb250ZW50IjpbIi8vIGV4dHJhY3RlZCBieSBtaW5pLWNzcy1leHRyYWN0LXBsdWdpbiJdLCJtYXBwaW5ncyI6IkFBQUEiLCJzb3VyY2VSb290IjoiIn0=\n//# sourceURL=webpack-internal:///./src/main/js/add-item.less\n");

/***/ }),

/***/ 3:
/*!*******************************************************************!*\
  !*** multi ./src/main/js/add-item.js ./src/main/js/add-item.less ***!
  \*******************************************************************/
/*! no static exports found */
/***/ (function(module, exports, __webpack_require__) {

__webpack_require__(/*! C:\Dev\Git\jenkins\war\src\main\js\add-item.js */"./src/main/js/add-item.js");
module.exports = __webpack_require__(/*! C:\Dev\Git\jenkins\war\src\main\js\add-item.less */"./src/main/js/add-item.less");

This could be problematic when running in Docker.

@95jonpet
Copy link
Contributor Author

I can see them when running the built PR in Docker using the following command:

docker run --rm -ti -p 8080:8080 -e ID=4753 jenkins/core-pr-tester

image

The "base-styles-v2.less:99" reference works. Clicking on it reveals the .less file:

image

@oleg-nenashev oleg-nenashev requested a review from timja May 29, 2020 13:01
@timja
Copy link
Member

timja commented May 29, 2020

Weird I tested this again in development and production mode and I still don't see source maps for less =/

@fqueiruga
Copy link
Contributor

Me neither

@timja
Copy link
Member

timja commented May 29, 2020

@95jonpet would you be able to see if it works for you in a local setup please?

Here's the documentation for frontend work:
https://github.com/jenkinsci/jenkins/blob/master/CONTRIBUTING.md#building-frontend-assets

@95jonpet
Copy link
Contributor Author

Sure thing.

@95jonpet
Copy link
Contributor Author

@timja, thank you for linking relevant documentation.

I have changed implementation during development to inline-cheap-module-source-map. They work when testing your scenario. My previous "solution" messed up the includes and only listed the final .less-files such as base-styles-v2.less. This is now fixed, resulting in the includes, such as typography.less being shown instead.

image

Clicking on typography.less reveals the source code:

image

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Thanks, I've tested in both production and development mode and it's working now

@timja timja added internal developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels May 30, 2020
@timja
Copy link
Member

timja commented May 30, 2020

This PR is now ready for merge, and will merged after ~24 hours if there's no negative feedback

@timja timja removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 30, 2020
@timja
Copy link
Member

timja commented May 30, 2020

Unmarking as ready-for-merge

I've been running this locally and after making a change it seems to crash my Mac,
I get a complete lock up of my machine, the only way to fix it is to reboot the mac, or very slowly get a terminal and kill the java process that's running at 800% CPU

I took a photo of the log from my phone as the computer was completely unresponsive:

mvnbuild

I was editing less files in development mode at the time,

Would be good to know if this is affecting anyone else before we merge this

@timja
Copy link
Member

timja commented May 30, 2020

I'm seeing similar-ish behaviour in the logs without this change but while my computer gets slower it doesn't fully lock up:

0 info it worked if it ends with ok
1 verbose cli [
1 verbose cli   '/Users/timja/projects/jenkinsci/jenkins/war/node/node',
1 verbose cli   '/Users/timja/.nvm/versions/node/v12.14.1/bin/npm',
1 verbose cli   'run',
1 verbose cli   'mvnbuild'
1 verbose cli ]
2 info using npm@6.13.4
3 info using node@v12.14.1
4 verbose stack Error: missing script: mvnbuild
4 verbose stack
4 verbose stack Did you mean this?
4 verbose stack     build
4 verbose stack     at run (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/lib/run-script.js:155:19)
4 verbose stack     at /Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/lib/run-script.js:63:5
4 verbose stack     at /Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:116:5
4 verbose stack     at /Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:436:5
4 verbose stack     at checkBinReferences_ (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:391:45)
4 verbose stack     at final (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:434:3)
4 verbose stack     at then (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:161:5)
4 verbose stack     at ReadFileContext.<anonymous> (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/read-package-json/read-json.js:350:20)
4 verbose stack     at ReadFileContext.callback (/Users/timja/.nvm/versions/node/v12.14.1/lib/node_modules/npm/node_modules/graceful-fs/graceful-fs.js:115:16)
4 verbose stack     at FSReqCallback.readFileAfterOpen [as oncomplete] (fs.js:239:13)
5 verbose cwd /Users/timja/projects/jenkinsci/jenkins/war
6 verbose Darwin 19.3.0
7 verbose argv "/Users/timja/projects/jenkinsci/jenkins/war/node/node" "/Users/timja/.nvm/versions/node/v12.14.1/bin/npm" "run" "mvnbuild"
8 verbose node v12.14.1
9 verbose npm  v6.13.4
10 error missing script: mvnbuild
10 error
10 error Did you mean this?
10 error     build
11 verbose exit [ 1, true ]

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 30, 2020
@timja
Copy link
Member

timja commented May 30, 2020

Seems to be fixed by either adding blueocean.features.BUNDLE_WATCH_SKIP or not having the blue ocean plugin installed in the development instance

https://github.com/jenkinsci/blueocean-plugin/blob/77960a8dc21840ae5f6df784a26f91e09d7da621/blueocean-web/src/main/java/io/jenkins/blueocean/dev/RunBundleWatches.java#L82

cc @halkeye if you've seen something similar before

@timja timja merged commit 179f8e3 into jenkinsci:master May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers internal ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants