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: Exclude all debug-like files from the build #11211

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

johncowen
Copy link
Contributor

Luckily I have a thing I was playing with that made me realise we had ballooned our vendor javascript file from about 430kb gzipped to about 1MB gzipped / 3.3MB un-gzipped (around double our normal size). Pretty sure I'd managed to import the whole of faker.js 😬 .

This PR adds **/*-debug.* to our test/prod excluded files (realised I needed to add test-support.js also so added that here as its more or less the same thing). Conditionally juggling ES6 static imports (specifically debug ones) for this was also getting a little hairy, so I moved it all to use the same approach as our conditional routes. All in all it brings the vendor build back down to ~430kb gzipped.

I wasn't sure whether to pop this on the end of #11188 but since thats already an umbrella issue, I decided to go with a separate PR (will add a new TODO over in 11188). Lastly, theres a little evolution here, and there's probably a tiny bit more cleanup and DRYing out to come, but it's fine to kick that can down the road for a little bit longer if that's preferable and put that in a separate PR.

@johncowen johncowen added the theme/ui Anything related to the UI label Oct 4, 2021
@johncowen johncowen requested a review from kaxcode October 4, 2021 17:29
const appNameJS = appName
.split('-')
.map((item, i) => (i ? `${item.substr(0, 1).toUpperCase()}${item.substr(1)}` : item))
.join('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocking comment/question, but curious to know if there are advantages to hand-rolling this as opposed to using something like @ember/string/classify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the main reason is this file gets sourced/exec'd before ember is even loaded, and it's in a separate file (that is very very small and quick to load). So we don't have any ember things available to use at this point, and loading anything ember related in this file would bloat it out quite a bit I think.

I thought it might be worth briefly going over some of the objectives here as an FYI:

  • Whether this file gets sourced or not should be entirely controllable, dynamically, from The Outside. Pretty much in our case this means from the backend (whether that is Consul in prod or ember-cli in dev) via the index.html file (which the backend can control)
  • We should be able to safely retrieve the data in this file from our javascript app (our/Ember's service container) without having to change that file. Control as to what services get loaded should be entirely the concern of the backend.
  • This file shouldn't pollute the global scope with variables and/or functions in enabling our/Ember's service container to be able to retrieve this data.
  • Ideally this file would be just JSON (or potentially something less verbose), but we need the tiniest bit of code to convert this JSON to a string and put it somewhere accessible to our/Ember's service container.

Right now we 'safely' move this data from this file into our/Ember's service container using the app name to namespace the script tags that get 'slurped' up by the our/Ember's service container (to ensure we only slurp the correct data for the app). I'm unsure right now if doing this extra detail/level of 'safety' is complete overkill, and its the reason for this bit of code. So it's highly likely that we'll stop doing this by the time this hits a proper release. We could also just hardcode the consulUi string that this produces, hardcoding here wouldn't be a major issue at all either. All in all I've not decided yet, would be good to hear what you thoughts there are also tho.

Copy link
Contributor

@evrowe evrowe 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 good to merge, given CI passes and the builds are producing the expected output

@evrowe
Copy link
Contributor

evrowe commented Oct 7, 2021

@johncowen I'd love to block out some time soon to go over:

  • The application's general architecture
  • The routing solution being used
  • History of decisions made around critical aspects of the architecture

@johncowen
Copy link
Contributor Author

Thanks for the review!

I'd love to block out some time soon to go over:

The application's general architecture
The routing solution being used
History of decisions made around critical aspects of the architecture

Yeah absolutely! Sounds good, I have a markdown file similar to #11203 in the works for these things already. Would be good to do that in tandem with a convo around this.

Ummm, in the meantime here're some good PRs/RFCs for some background:

I think those ^ give a fairly good overview of our entire application (apart from styling) and it what means we can write our application as just declarative HTML (well custom-elements more or less). There is some further more 'consumer' based documentation in our Consul UI Eng docs (the ones bundled with out app during dev)

Hopefully thats not too much overload rn! Lemme know when you want to block time out to chat through stuff 👍

@johncowen johncowen merged commit 42d7b98 into ui/feature/test-helpers Oct 8, 2021
@johncowen johncowen deleted the ui/chore/exclude-more-debug branch October 8, 2021 11:36
johncowen added a commit that referenced this pull request Oct 8, 2021
* Add `is` and `test` helpers in a similar vein to `can`

Adds 2 new helpers in a similar vein to ember-cans can:

- `is` allows you to use vocab/phrases such as (is "something model") which calls isSomething() on the models ability.
- `test` allows you to use vocab/phrases such as (test "is something model") or (test "can something model")which calls isSomething() / canSomething() on the models ability. Mostly using the is helper and the can helper. It's basically the is/can helper combined.

* Adds TextInput component + related modifiers/helpers/machines/services (#11189)

Adds a few new components/modifiers/helpers to aid building forms.

- state-chart helper, used in lieu of a more generic approach for requiring our statecharts.
- A few modifications to our existing disabled modifier.
- A new 'validation' modifier, a super small form validation approach built to make use of state charts (optionally). Eventually we should be able to replace our current validation approach (ember-changeset-validations + extra deps) with this.
- A new TextInput component, which is the first of our new components specifically to make it easy to build forms with validations. This is still a WIP, I left some comments in pointing out where this one would be progressed, but as we don't need the planned functionality yet, I left it where it was. All of this will be fleshed out more at a later date.

Documentation is included for all of ^

* ui: Adds initial CRUD for partitions (#11190)

Adds basic CRUD support for partitions. Engineering-wise probably the biggest takeaway here is that we needed to write very little javascript code to add this entire feature, and the little javascript we did need to write was very straightforwards. Everything is pretty much just HTML. Another note to make is that both ember-changeset and ember-data (model layer things) are now completely abstracted away from the view layer of the application.

New components:

- Consul::Partition::Form
- Consul::Partition::List
- Consul::Partition::Notifications
- Consul::Partition::SearchBar
- Consul::Partition::Selector

See additional documentation here for more details

New Route templates:

- index.hbs partition listing/searching/filtering
- edit.hbs partition editing and creation

Additionally:

There is some additional debug work here for better observability and to prevent any errors regarding our href-to usage when a dc is not available in our documentation site.

Our softDelete functionality has been DRYed out a little to be used across two repos.

isLinkable was removed from our ListCollection component for lists like upstream and service listing, and instead use our new is helper from within the ListCollection, meaning we've added a few more lighterweight templateOnly components.

* ui: Exclude all debug-like files from the build (#11211)

This PR adds **/*-debug.* to our test/prod excluded files (realised I needed to add test-support.js also so added that here as its more or less the same thing). Conditionally juggling ES6 static imports (specifically debug ones) for this was also getting a little hairy, so I moved it all to use the same approach as our conditional routes. All in all it brings the vendor build back down to ~430kb gzipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants