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

Clarify calling one library in two ways on 'Effective Dart: Usage' page #3124

Closed
dark-chocolate opened this issue Apr 6, 2021 · 11 comments · Fixed by #5779
Closed

Clarify calling one library in two ways on 'Effective Dart: Usage' page #3124

dark-chocolate opened this issue Apr 6, 2021 · 11 comments · Fixed by #5779
Assignees
Labels
a.effective-dart Relates to the best practices explained in Effective Dart e1-hours Can complete in < 8 hours of normal, not dedicated, work p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. st.triage.ltw Indicates Lead Tech Writer has triaged

Comments

@dark-chocolate
Copy link
Contributor

Page URL: https://dart.dev/guides/language/effective-dart/usage
Page source: https://github.com/dart-lang/site-www/tree/master/src/_guides/language/effective-dart/usage.md

Description of issue:

Quoting this article,

image

I think the first line is not bad, but actually is the right way to import. It is further confirmed in the next section:

image

@kwalrath
Copy link
Contributor

kwalrath commented Apr 9, 2021

It's true that the first line isn't bad; it's the combination of the two lines that's bad. I'm not sure how to make this clearer.

@dark-chocolate
Copy link
Contributor Author

@kwalrath By "combination", do you mean there should be a space between the two lines (as mentioned in best practices)?

@kwalrath
Copy link
Contributor

kwalrath commented Apr 9, 2021

No, I mean that you're including the same library in two different ways, which is very bad.

Maybe we could change the example to something like this:

// This import by itself would be fine.
import 'package:my_package/api.dart';

// This import causes problems because it imports the same file (api.dart)
// but uses a different path. Avoiding ../lib imports prevents this problem. 
import '../lib/api.dart'; // Reaching into lib.

@kwalrath kwalrath added a.effective-dart Relates to the best practices explained in Effective Dart act.wait-for-customer Needs response from customer labels Apr 9, 2021
@kwalrath
Copy link
Contributor

kwalrath commented Apr 9, 2021

/cc @munificent in case he has suggestions.

@dark-chocolate
Copy link
Contributor Author

@kwalrath I didn't pay attention to the 2nd import line in 1st screenshot. I thought it would be import 'test_utils.dart'. I got your point now.

@kwalrath
Copy link
Contributor

kwalrath commented Apr 9, 2021

@kwalrath I didn't pay attention to the 2nd import line in 1st screenshot. I thought it would be import 'test_utils.dart'. I got your point now.

You aren't the first person to be confused by the example, so I think it needs some improvement.

@kwalrath
Copy link
Contributor

Turns out we had a PR from long ago that aims to fix this. I think it's ready to commit, and I hope it helps.

@parlough
Copy link
Member

parlough commented Sep 1, 2021

I think the addition of a "good" example with the current text after #3066 seems to sufficiently solve this issue. Let me know if you think further additions or changes are needed.

Thanks again for creating an issue for this!

@parlough parlough closed this as completed Sep 1, 2021
@dark-chocolate
Copy link
Contributor Author

Hi,

It's still confusing to have first import line (which is good) in the bad section.

image

I think it will make more sense if we can write it like this:

api.dart can be imported in two ways:

  1. import '../lib/api.dart'; (bad)
  2. import 'package:my_package/api.dart'; (good)

@parlough
Copy link
Member

Hi @dark-chocolate, thanks for raising the concern. Taking a look, this snippet is not about the first one being good and the second one being bad, it's about how importing in both ways at the same time is very confusing and causes issues. So the suggestion to this issue, is to just stick to the package one.

If we only put the import '../lib/api.dart'; in the bad section, it doesn't illustrate the potential ("bad") issues that may arise.

Keeping that in mind, do you see someway this would be more clear?

@dark-chocolate
Copy link
Contributor Author

@parlough

It's not clear what is bad.

import '../lib/api.dart';

or the combination of

import 'package:my_package/api.dart';
import '../lib/api.dart';

Of course both are bad. I think the docs needs to convey the following message explicitly because the reader also knows that importing both will be bad, the doc was (I think) more about what should be the correct way of importing.

it's about how importing in both ways at the same time is very confusing and causes issues

@parlough parlough reopened this Oct 21, 2022
@parlough parlough added p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. e0-minutes Can complete in < 60 minutes of normal, not dedicated, work e1-hours Can complete in < 8 hours of normal, not dedicated, work and removed act.wait-for-customer Needs response from customer labels Oct 21, 2022
@parlough parlough self-assigned this Oct 21, 2022
@atsansone atsansone changed the title 'Effective Dart: Usage' page issue Clarify calling one library in two ways on 'Effective Dart: Usage' page Apr 5, 2023
@atsansone atsansone added st.triage.ltw Indicates Lead Tech Writer has triaged and removed e0-minutes Can complete in < 60 minutes of normal, not dedicated, work labels Apr 5, 2023
parlough added a commit that referenced this issue May 29, 2024
Use 11ty's first-party [image transformation
plugin](https://www.11ty.dev/docs/plugins/image/) to automatically
optimize images, convert them to `png`, `webp`, and `avif`, and then
transform the site HTML to use the [`<picture>`
element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture)
to select the best one for the user. This only runs in production deploy
builds to reduce serving time. Also applies lazy loading and async
decoding to all images, even when serving without optimizations. The
transform also adds a hash of the image, allowing us to expand caching
for images in Firebase hosting.

To account for the transformed HTML structure, some minor HTML and CSS
changes were needed as well.

Overall, this reduces page load time on pages that use images, and
reduces unnecessary downloads that were due to a relatively short cache
time for images. On a page with just a few images, such as
https://dart.dev/tools/pub/automated-publishing, the lighthouse perf
score increases around 10-15 points.

Fixes #4473
Fixes #3124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a.effective-dart Relates to the best practices explained in Effective Dart e1-hours Can complete in < 8 hours of normal, not dedicated, work p3-low Valid but not urgent concern. Resolve when possible. Encourage upvote to surface. st.triage.ltw Indicates Lead Tech Writer has triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants