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

Document conditional imports #2079

Merged
merged 14 commits into from
Nov 13, 2019
Merged

Document conditional imports #2079

merged 14 commits into from
Nov 13, 2019

Conversation

kwalrath
Copy link
Contributor

@kwalrath kwalrath commented Nov 8, 2019

I put most of the content in the "Creating library packages" page, not directly in the language tour.

Staged:
https://kw-staging-dartlang-2.firebaseapp.com/guides/language/language-tour#implementing-libraries
https://kw-staging-dartlang-2.firebaseapp.com/guides/libraries/create-library-packages#conditionally-importing-and-exporting-library-files

I'd love some feedback on the most common use cases (e.g. which platforms you'd support, and if there's something else I should show/cover).

Fixes #1569

@kwalrath kwalrath requested review from jonasfj and mit-mit November 8, 2019 01:53
@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Nov 8, 2019
@mit-mit
Copy link
Member

mit-mit commented Nov 8, 2019

cc @lrhn can you give this a review, please?

@jonasfj jonasfj requested a review from lrhn November 8, 2019 15:31
Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

I defer to @lrhn for this, but looks decent to me.

examples/create_libraries/lib/src/hw.dart Outdated Show resolved Hide resolved
then export `hw_io.dart`.
* If this code is running in a browser,
then export `hw_html.dart`.
* Otherwise, export `hw.dart`. **[PENDING: when would this happen?]**
Copy link
Member

Choose a reason for hiding this comment

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

I think the analyzer will import this during development, so that might be where code completion comes from (check if this is true).

Also if one day a platform that supports neither dart:html or dart:io then I guess it would be imported there. Though no such platform currently exist.

To conditionally import or export,
you must check for the presence of `dart:*` libraries.
Here's an example of conditional export code that
checks for the presence of `dart:io` and `dart:html`:
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would perhaps consider testing on dart:js instead of dart:html because I would imagine dart:js is more comon in Flutter for web packages (just a guess).

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've seen dart:html in the wild, but that's probably pre-flutter web, and I'd love to get a definitive answer on this. @johnpryan?

Copy link
Member

Choose a reason for hiding this comment

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

It depends on what platform library you actually depend on.
This is library testing, not platform testing—which is why the example is actually not very good. It's not detecting "VM" or "JavaScript", it's detecting the presence of the dart:io library or the dart:html library.

I know of at least one experiment with embedding the VM and a browser engine in the same program, and that platform hae both dart:io and dart:html (not sure if it had dart:js though, since you used Dart instead).

So, if you depend on dart:js then test for dart.library.js, and if you depend on dart:html then test for dart.library.html. That makes your code maximally supported.

In most cases, the feature you need is a DOM feature, not a plain JS feature, so I'd test for dart.library.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if both dart:io & dart:html are supported? Would only the first if take effect, or might you import multiple libraries? (I'd hope it's the former, not the latter.)

examples/create_libraries/lib/src/hw.dart Outdated Show resolved Hide resolved
To conditionally import or export,
you must check for the presence of `dart:*` libraries.
Here's an example of conditional export code that
checks for the presence of `dart:io` and `dart:html`:
Copy link
Member

Choose a reason for hiding this comment

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

It depends on what platform library you actually depend on.
This is library testing, not platform testing—which is why the example is actually not very good. It's not detecting "VM" or "JavaScript", it's detecting the presence of the dart:io library or the dart:html library.

I know of at least one experiment with embedding the VM and a browser engine in the same program, and that platform hae both dart:io and dart:html (not sure if it had dart:js though, since you used Dart instead).

So, if you depend on dart:js then test for dart.library.js, and if you depend on dart:html then test for dart.library.html. That makes your code maximally supported.

In most cases, the feature you need is a DOM feature, not a plain JS feature, so I'd test for dart.library.html.

examples/create_libraries/lib/src/hw_io.dart Show resolved Hide resolved
examples/create_libraries/example/hw_example.dart Outdated Show resolved Hide resolved
@kwalrath
Copy link
Contributor Author

ptal

@kwalrath kwalrath changed the title [WIP] Document conditional imports Document conditional imports Nov 12, 2019
@kwalrath kwalrath removed the ⚠ WIP label Nov 12, 2019
@kwalrath
Copy link
Contributor Author

@lrhn still lgty?

@kwalrath
Copy link
Contributor Author

I'm taking that as a yes. :)

@kwalrath kwalrath merged commit d315af7 into master Nov 13, 2019
@kwalrath kwalrath deleted the kw-condl-imports branch November 13, 2019 18:45
@@ -0,0 +1,5 @@
import 'package:create_libraries/hw_mp.dart';

main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kwalrath: main() -> void main() (to avoid an analyzer issue)?

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I added void in a recent PR, matching how we declared the alarm() function. If we've decided that to promote the declaration of main() w/o it's type, I'll revert the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional imports is not covered in the language tour
6 participants