Conversation
|
@armiol, PTAL. |
Codecov Report
@@ Coverage Diff @@
## master #524 +/- ##
============================================
- Coverage 74.16% 73.71% -0.46%
- Complexity 2935 2936 +1
============================================
Files 502 503 +1
Lines 11787 11869 +82
Branches 661 671 +10
============================================
+ Hits 8742 8749 +7
- Misses 2823 2898 +75
Partials 222 222 |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmdashenkov please see my comments.
| */ | ||
|
|
||
| package io.spine.js.generate.resolve; | ||
| package io.spine.tools.code; |
There was a problem hiding this comment.
According to the class-level comment, this is a JavaScript module. However, as you move it out from the JS-specific package, it no longer looks as such. NPM reference is also misleading to me.
| private final DirectoryProperty testGeneratedDir; | ||
|
|
||
| /** | ||
| * Names of JavaScript modules and directories they provide. |
There was a problem hiding this comment.
I don't understand why it is a JavaScript module you speak about.
| * modules = [ | ||
| * // The module provides `company/client` directory (not including subdirectories). | ||
| * // So, an import path like {@code ../company/client/file.js} | ||
| * // becomes {@code client/company/client/file.js}. |
There was a problem hiding this comment.
And .js files in the reference.
| } | ||
|
|
||
| /** | ||
| * Reads the file from disc. |
There was a problem hiding this comment.
I think it's from "a local filesystem".
| void resolveImports(ImmutableList<ExternalModule> modules, Path libPath) { | ||
| lines = lines.stream() | ||
| .map(line -> resolveImportInLine(line, modules, libPath)) | ||
| .collect(toList()); |
There was a problem hiding this comment.
This one is confusing. So you could just call the method for each of the modules, but instead, you are collecting them into a redundant list to invoke the methods.
|
|
||
| @CheckReturnValue | ||
| @ParametersAreNonnullByDefault | ||
| package io.spine.js.generate.imports; |
There was a problem hiding this comment.
Please document the package.
| return new ResolveImports(generatedProtoDir, ImmutableList.of(module)); | ||
| } | ||
|
|
||
| private static ExternalModule newModule(String moduleName, String directoryPattern) { |
There was a problem hiding this comment.
It's weird that this file has just this one private static method appended. Please review.
| @ParametersAreNonnullByDefault | ||
| package io.spine.js.generate.imports.given; | ||
|
|
||
| import com.google.errorprone.annotations.CheckReturnValue; |
There was a problem hiding this comment.
Please document this package, for consistency sake.
|
@armiol, PTAL again. |
2 participants
The problem
Dart, just like JavaScript, has a file-based import system (as opposed to class-based in Java).
When compiling Protobuf into Dart, we have to compile all the accessible
.protofiles, including those from the project's dependencies. Otherwise, the compiled files would have broken imports pointing at third-party types which are not there.Example
Module
my-appuses librarymy-lib. Bothmy-appandmy-libdeclare Protobuf types and add utilities for working with those types.If
my-apptypes usemy-libtypes, the imports generated inmy-appwill point to relative paths where themy-libgenerated Dart classes should be. So, if we don't compilemy-libtypes inmy-app,my-apptypes contain broken imports.Solution
Just like in JS, we fix the imports in the generated Dart code. For that, we require the user to construct a map of all the known Dart modules with generated code to their Protobuf packages (or package prefixes).