Support multiple source directories #1

sentience · 2017-03-22T04:42:31Z

elm-package.json may specify multiple source directories for a single Elm project (e.g. application modules and library modules). Modules in one source directory may import modules in another source directory transparently.

With this change, we now find the elm-package.json file responsible for the requested module and use it to search all of the project’s source directories for each dependency.

elm-package.json may specify multiple source directories for a single Elm project (e.g. application modules and library modules). Modules in one source directory may import modules in another source directory transparently. With this change, we now find the elm-package.json file responsible for the requested module and use it to search all of the project’s source directories for each dependency.

sentience · 2017-03-22T04:50:37Z

We’re hoping to get this merged and released in elm-webpack-loader (via node-elm-compiler), so that we can use webpack --watch reliably with our multi-source-dir project.

stoeffel

Thanks for your PR ❤️. Code looks great. I had just one minor comment.

stoeffel · 2017-03-22T21:02:37Z

index.js

  }
 }

+// Given a source directory (containing top-level Elm modules), locate the
+// elm-package.json file that includes it.
+function findElmPackage(baseDir, currentDir) {


🎨 we can drop currentDir and just use one arg.

My intent here was to preserve baseDir through the recursion so that when we found the elm-package.json file, we could verify that the baseDir in question was actually listed in its source-directories, but I ended up not writing this check. What do you think? Would this level of robustness be useful? Or is it sufficient just to search up through parent directories and use the first elm-package.json file that we find (in which case I will delete the unneeded variable).

Thinking about it some more, I’m thinking we probably should check that the source-directories does contain baseDir. If not, and the elm-package.json contains source directories that don’t match, find-elm-dependencies is going to be unable to find any dependencies in that baseDir, and it will be really confusing.

I’ll update this and include a test case.

sentience · 2017-03-23T01:04:35Z

@stoeffel OK, should be good to go now.

sentience · 2017-03-23T05:20:45Z

Important to note this changes the API signature for the exported findAllDependencies function (sourceDirectories takes an array whereas baseDir took a string), so if you’re doing Semver you’ll need to increment the major version for this change. I assumed no one was depending on that argument’s type at this point.

stoeffel · 2017-03-23T08:19:04Z

@sentience The cli doesn't work anymore.

$ ./bin/cli.js ./test/fixtures/Parent.elm
[]

Would you mind fixing that? Let me know if you need help.

stoeffel · 2017-03-23T08:21:22Z

index.js

  if (typeof currentDir === "undefined") {
    currentDir = baseDir;
  }

  var elmPackagePath = path.join(currentDir, 'elm-package.json');
  if (fs.existsSync(elmPackagePath)) {
-    return elmPackagePath;
+    var sourceDirectories = getSourceDirectories(elmPackagePath);
+    if (_.includes(sourceDirectories, baseDir)) {


I'm worried that this never halts, when this isn't true. I think this should raise an error. I also think that the source-directories don't have to include base dir right? You could have a elm-package with only ./src.

It will halt when the search for a matching elm-package.json reaches the root directory of the file system (see return false; on line 89).

Basically we ignore the “bad” elm-package.json file and continue searching up the path for a “good” elm-package.json file. If we never find one, we give up when we get to the filesystem root.

Hope that makes sense!

Makes sense, but:

I also think that the source-directories don't have to include base dir right? You could have a elm-package with only ./src.

Correct, and that’s supported by my implementation already. If you have:

my-package `- elm-package.json ("source-directories": ["./src"]) `- src `- Main.elm (module Main exposing (..))

…then getBaseDir('/path/to/src/Main.elm') will return /path/to/src, and then getElmPackageSourceDirectories('/path/to/src') will find elm-package.json in the parent directory, confirm it includes ./src in "source-directories" and return ['/path/to/src'].

Unless I’m missing something?

Oh, you're right!

sentience · 2017-03-23T08:56:11Z

Taking a look at the CLI now.

sentience · 2017-03-23T09:19:53Z

CLI is fixed. Good catch!

stoeffel · 2017-03-23T15:06:11Z

Thanks for your contribution.

stoeffel · 2017-03-23T15:11:42Z

published with v1.0.0

eeue56 · 2017-03-24T13:28:14Z

index.js

+// elm-package.json file that includes it and get all its source directories.
+function getElmPackageSourceDirectories(baseDir, currentDir) {
+  if (typeof currentDir === "undefined") {
+    currentDir = baseDir = path.resolve(baseDir);


Please set this on multiple lines, this is prone to error.

eeue56 · 2017-03-24T13:28:56Z

index.js

+  }
+
+  if (isRoot(currentDir)) {
+    return false;


What is the type of this function meant to be? Make it return an empty list instead of a bool.

Good catch!

eeue56 · 2017-03-24T13:29:56Z

index.js


-function findAllDependenciesHelp(file, knownDependencies, baseDir, knownFiles) {
+function getSourceDirectories(elmPackagePath) {
+  var elmPackage = JSON.parse(fs.readFileSync(elmPackagePath, 'utf8'));


this will crash if there is invalid json, with a bad error message. Good to catch it and return an empty list instead.

eeue56 · 2017-03-24T13:31:16Z

index.js

+          _.find(sourceDirectories, function(sourceDir) {
+            var absPath = path.join(sourceDir, dependencyLogicalName + extension);
+            if (fs.existsSync(absPath)) {
+              result = absPath;


why use mutation? Do result = _.find(...)

The mutation avoids having to either map all entries to absolute paths, or having to map the found result a second time after returning it. I'm happy to go with either one if you consider this an unwanted optimisation.

I'd rather spend milliseconds on avoiding crashes than hours debugging :)

eeue56 · 2017-03-24T13:32:37Z

@sentience I have some concerns with the code that I'd like addressed before using the webpack-loader to rely on this. I've commented on the diff.

sentience · 2017-03-24T13:35:53Z

Thanks for the review! I'll open a second PR with the requested improvements to release as 1.0.1.

sentience added 2 commits March 22, 2017 12:48

Remove unused functions

ed88888

stoeffel self-requested a review March 22, 2017 20:56

stoeffel reviewed Mar 22, 2017

View reviewed changes

Ensure elm-package.json source-directories include the module

7212195

stoeffel reviewed Mar 23, 2017

View reviewed changes

Fix support for relative path on input

9ee0ccf

stoeffel merged commit 873f939 into NoRedInk:master Mar 23, 2017

sentience deleted the multiple-src-dirs branch March 23, 2017 23:34

eeue56 reviewed Mar 24, 2017

View reviewed changes

sentience mentioned this pull request Mar 25, 2017

Multiple source directories code review #3

Merged

rtfeldman pushed a commit that referenced this pull request Aug 28, 2018

Replace test references to old elm-package.json with elm.json (#1)

f6c758c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple source directories #1

Support multiple source directories #1

sentience commented Mar 22, 2017

sentience commented Mar 22, 2017 •

edited

Loading

stoeffel left a comment

stoeffel Mar 22, 2017

sentience Mar 22, 2017

sentience Mar 22, 2017

sentience commented Mar 23, 2017

sentience commented Mar 23, 2017

stoeffel commented Mar 23, 2017

stoeffel Mar 23, 2017

sentience Mar 23, 2017

stoeffel Mar 23, 2017

sentience Mar 23, 2017 •

edited

Loading

stoeffel Mar 23, 2017

sentience commented Mar 23, 2017

sentience commented Mar 23, 2017

stoeffel commented Mar 23, 2017

stoeffel commented Mar 23, 2017

eeue56 Mar 24, 2017

eeue56 Mar 24, 2017

sentience Mar 24, 2017

eeue56 Mar 24, 2017

eeue56 Mar 24, 2017

sentience Mar 24, 2017

eeue56 Mar 24, 2017

sentience Mar 24, 2017

eeue56 commented Mar 24, 2017

sentience commented Mar 24, 2017

Support multiple source directories #1

Support multiple source directories #1

Conversation

sentience commented Mar 22, 2017

sentience commented Mar 22, 2017 • edited Loading

stoeffel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentience commented Mar 23, 2017

sentience commented Mar 23, 2017

stoeffel commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentience Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentience commented Mar 23, 2017

sentience commented Mar 23, 2017

stoeffel commented Mar 23, 2017

stoeffel commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eeue56 commented Mar 24, 2017

sentience commented Mar 24, 2017

sentience commented Mar 22, 2017 •

edited

Loading

sentience Mar 23, 2017 •

edited

Loading