Fix multiple copies of preact in production build (fix #126)

[reznord]

Fix for #126

[rkostrzewski]

Travis build failed because we Skip installation of dependencies in tests. I'll look into enabling it & caching.

[reznord]

@rkostrzewski the tests failed in my local machine as well. Can you please have a look at the log?

https://paste.fedoraproject.org/paste/~djRyBcRUltRzRRaON3~qg

[developit]

I wonder if falling back to require.resolve() if the file doesn't exist would work here

[reznord]

not sure how that is gonna fit in. Can you tell me how to get this problem fixed?

[lukeed]

I think we just need to provide a path to the dev/module bundle? We can do this at the top of the config... something like:

let preact, type = isProd ? 'preact/dist/preact.min' : 'preact';

try {
  preact = require.resolve(type);
} catch (err) {
  throw err;
}

// ...

preact$: preact

Also, this is because I'm suspecting that env.cwd isn't being set correctly.

---

Not able to go thru this right now & debug what's happening, else I would! 😄

[rkostrzewski]

Nah, tests just skip npm install step to be faster.

@reznord the easiest way is to remove --no-install from here tough I'm afraid timeout will occur. I'll tinker around this tomorrow hopefully.

[reznord]

Can't we use yarn? It is much faster compared to npm. I know this is just going towards #15 😅

[rkostrzewski]

More like #31 😃

[reznord]

@rkostrzewski the suggestion by @lukeed works perfectly fine. Though that is more like a hack. If it is fine, I'll make another PR to the same with the patch by @lukeed.

I hope that will be fine.

[developit]

Suggestion - we can use require-relative for this:

import requireRelative from 'require-relative';

// attempt to resolve a dependency, giving $CWD/node_modules priority:
function resolveDev(dep, cwd) {
  try { return requireRelative.resolve(dep, cwd || process.cwd()); } catch (e) {}
  try { return require.resolve(dep); } catch (e) {}
  return dep;
}

// ...
aliases: {
  preact$: resolveDep(isProd ? 'preact/dist/preact.min' : 'preact', env.cwd),
}

[lukeed]

Yeah, we can. Is there a benefit here though? Plus I still have a feeling that env.cwd isn't set correctly

[developit]

Hmm. If env.cwd isn't set, a lot of things should be blowing up. It should point to the directory where preact-cli was spawned, or the custom directory passed to the CLI in the case of tests and whatnot (ref).

The reason I suggested require-relative is so that if someone installs a specific version of preact or preact-compat, they'll get that version. I want to avoid people being stuck with whatever version we happened to ship with preact-cli. We'll want to take that into account when doing prerendering too - currently that uses the one we ship with as well.

[lukeed]

Yeah, that's what I thought, but was having doubts looking at the travis logs here.

I agree. But in a Node 6+ environment, it's flat dep tree, so wouldn't a custom install overwrite what we ship? I'm inclined to think it does, since it's the same name (as opposed to a unique dep), but today also seems to be "just one of those days". 😴 😇

[developit]

npm link creates non-flat trees, I actually think that might have been what @reznord was seeing originally. Interestingly, this webpack config option emulates kinda what I was going for with the cwd-first approach.

[reznord]

when I run npm run test it is opening up Google Chrome Canary. Any idea why that is happening?

[reznord]

In the travis log, this kinda caught my mind (https://travis-ci.org/developit/preact-cli/jobs/246455525#L1827) any idea why there are two instances of that?

[rkostrzewski]

@reznord it's opening Google Chrome because tests use it to verify if the app works correctly 😄. As for the warning with AudioManager - it doesn't matter at all (folks at lighthouse have that warning too).

[reznord]

@rkostrzewski cool. I have updated the PR. Can you please review it? As @developit suggested, I have added the require-resolve module and got it working.

[rkostrzewski]

LGTM!