build icon indicating copy to clipboard operation
build copied to clipboard

Cannot serve built to cache files in nested test directory

Open corwinsheahan-wf opened this issue 6 years ago • 5 comments

Hi there!

We noticed an issue crop up where css files built to cache via the sass_builder could not be found in the context of pub run build_runner test. When attempting to load built css files with a relative path from a custom test html file, those files were not found. After digging in further, we discovered that this is because the location of the html file was nested inside of the test dir at test/unit/. This problem did not manifest for files with a similar relative path which were present in source. I've created a simple project which exemplifies this issue here.

For quick reference, the main_test.html file is nested at test/unit, and looks like this (Note that will-load.css is not built by the sass_builder and is already present in source):

<!DOCTYPE html>
<html>
  <head>
    <title>generated_runner_test</title>
    <link rel="stylesheet" href="packages/built_to_cache_test/will-load.css">
    <link rel="stylesheet" href="packages/built_to_cache_test/will-not-load-built.css">
    <link rel="stylesheet" href="../packages/built_to_cache_test/will-load-built.css">
    <link rel="x-dart-test"  href="main_test.dart">
    <script src="packages/test/dart.js"></script>
  </head>
  <body></body>
</html>

And the test looks like this:

@TestOn('browser')
import 'dart:html';
import 'package:test/test.dart';

void main() {
  test('loading of css', () {
    final willLoadElement = document.createElement('div')..className = 'will-load-class';
    final willLoadBuiltElement = document.createElement('div')..className = 'will-load-built-class';
    final willNotLoadBuiltElement = document.createElement('div')..className = 'will-not-load-built-class';

    document.body.append(willLoadElement);
    document.body.append(willLoadBuiltElement);
    document.body.append(willNotLoadBuiltElement);

    // Will pass
    expect(willLoadElement.getComputedStyle().fontSize, '1px');
    // Will pass
    expect(willLoadBuiltElement.getComputedStyle().fontSize, '1px');
    // Will not pass, because css file cannot be found
    expect(willNotLoadBuiltElement.getComputedStyle().fontSize, '1px');
  });
}

I'm not sure if this is a bug, or if it's intentional, but it is at least confusing that the behavior is different between built files and files present in the filesystem. As a consumer of sass_builder, I would expect the behavior to be the same in this case.


Relevant dependency Information:

dart --version: Dart VM version: 2.2.0 (Tue Feb 26 15:04:32 2019 +0100) on "macos_x64"
build: 1.1.3
build_config: 0.3.2
build_daemon: 0.5.0
build_modules: 1.0.10
build_resolvers: 1.0.4
build_runner: 1.3.3
build_runner_core: 3.0.3
build_test: 0.10.7
build_web_compilers: 1.2.1"
sass_builder: 2.1.2

corwinsheahan-wf avatar Apr 24 '19 17:04 corwinsheahan-wf

Interesting - in general it is best to always reference the implicit packages directory that is directly under the first top level dir. I am actually surprised that <link rel="stylesheet" href="packages/built_to_cache_test/will-load.css"> works

jakemac53 avatar Apr 24 '19 21:04 jakemac53

Hmm, that is interesting. We've used this "feature" quite extensively 😄

corwinsheahan-wf avatar Apr 25 '19 14:04 corwinsheahan-wf

Just for some context, regardless of build system support, you always want to reference resources through the exact same uri for caching purposes. The easiest way to ensure that is to always reference the top level packages dir.

So for instance subdir/packages/foo/foo.css and packages/foo/foo.css are different files as far as the browser is concerned even if they are the same file in practice, and if they were both referenced in the same app then it would download the same css twice.

jakemac53 avatar Apr 25 '19 14:04 jakemac53

Thanks for the clarification! I think we're satisfied if y'all wanna close this issue.

corwinsheahan-wf avatar Apr 25 '19 15:04 corwinsheahan-wf

I think we should leave it open as this does sound like a legitmate bug :)

I believe we actually don't create nested packages dirs at all when we create output directories, but our development server supports arbitrarily nested packages dirs. So, at a minimum we should unify things there.

jakemac53 avatar Apr 25 '19 16:04 jakemac53