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

ui: Allow disabling of sourcemaps via env var #10491

Merged
merged 1 commit into from
Jul 6, 2021

Conversation

johncowen
Copy link
Contributor

The rebuild of the Consul UI used very minimal ES6 features for 2 reasons:

  1. Ember at the time felt very ES5-like (we started on Ember 2.18), to a certain extent there are still things in Ember that feel ES5-like (no real support for non-syntax related ES6 features like Sets and Maps in handlebars templates for example)
  2. Using less layers makes things simpler in lots of subtle ways, for example we didn't even realize that we had sourcemaps disabled for such a long time before ui: enable Babel source maps #7651

We had a period of time that we were we started moving to use more ES6 style syntax, but pretty soon after that we reduced our browser support (#9729) by not supporting anything older than ~2016. This brought us back to being able transpile as little as possible (decorators are so necessary for ember work we can't really avoid having to transpile those until they are native), and thus removing extra layers again. This means that sourcemaps aren't that much of a necessity again (depending on what your preference is)

Additionally, for a good while now I've noticed that since we enabled sourcemaps, they don't really work correctly in Ember. I often find myself debugging something and the stack trace line numbers are completely wrong, often reporting line numbers of over 1000 when the file where the error is is only a few lines long, it even sometimes gives me minus numbers. This can sometimes make chasing down errors a little frustrating as it is essentially saying 'the error is....umm I dunno, somewhere in this file but I'm not sure where'.

Here's one I noticed today which points me to line 1336 in a file of only 85 lines.

Screenshot 2021-06-24 at 12 39 29

And another from a few days ago telling me the error was in a comment

Screenshot 2021-06-21 at 16 45 28

I've also heard both online and offline that there are problems with sourcemaps and ember, potentially related to ember-auto-import, and this might be the root of the problem. Its a strange one, as you don't notice at first, and the line numbers seem fine to begin with, but as time goes on the line numbers seem to get wildly out of kilter.

I've experimented a little with turning sourcemaps off again and I no longer seem to have problems with line numbers and my browser points me to exactly where the error is within the file. As we aren't really doing much transpilation, this is much easier for me to read and figure out what is happening.

Therefore (for the moment at least) I personally prefer not to have sourcemaps enabled, but I understand other folks might not. This PR makes enabling/disabling sourcemaps possible via a local environment variable, so I can choose what is best for me whilst not affecting anyone else and not have to remember to not commit change in our config files that I use just for myself.

We can revisit this when the new ember build tooling is ready for prime time, but if you don't set the environment variable then there is no real change here.

@johncowen johncowen added theme/ui Anything related to the UI backport/1.10 labels Jun 24, 2021
@johncowen johncowen requested a review from kaxcode June 24, 2021 18:25
@mikemorris
Copy link
Contributor

mikemorris commented Jun 24, 2021

This does sound like a bug upstream in Ember or source map generation tooling somewhere (looks like line numbers are pointing to the transpiled file but inspector is showing the pre-transpilation source in these cases) that should be fixed rather than just losing this functionality, but local env var to optionally disable feels like an okay middle ground for now 👍

@johncowen johncowen added the pr/no-changelog PR does not need a corresponding .changelog entry label Jun 25, 2021
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

LGTM

@johncowen johncowen merged commit b0d69ef into main Jul 6, 2021
@johncowen johncowen deleted the ui/chore/build-improvements branch July 6, 2021 15:57
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/404161.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit b0d69ef onto release/1.10.x succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-changelog PR does not need a corresponding .changelog entry theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants