many fixes #27

pjordaan · 2017-10-31T11:30:04Z

fixes for packages that do define and calling the factory directly instead (for example zenscroll).

define(['dependencies'], factory()});

wrong module name for external modules with 'src' somewhere in the path. (for example angular2-swiper/src/ks-swiper.module)

Possible XSS on module name.

Do not automatically remove . from a module name, it's not always '.ts'

Enable ts-helpers to reuse ts transpile helper functions.

Enable emitting deocrator metadata for Angular DI.

Added optional config for FileConfig to provide non-static options.

Ignore comments with require() statements.

Make angular templates always relative and let the import provide the full path instead.

Parse less to css for angular templates possiblity.

Ignore .d.ts entirely. This is because some imports fail because end up at the definition file, while a source file is available too. For example @angular/platform-browser/animations resolved to node_modules/@angular/platform-browser/animations/index.d.ts, while it should resolve to node_modules/@angular/platform-browser/bundles/platform-browser-animations.umd.js by reading the package.json.

yannickl88 · 2017-10-31T11:39:06Z

src/Bundler/Processor/TsContentProcessor.php

-        if (false !== ($i = strrpos($module_name, '.'))) {
-            $module_name = substr($module_name, 0, $i);
+        if (preg_match('/\.ts$/i', $module_name)) {
+            $module_name = preg_replace('/\.ts$/i', '', $module_name);


can you not always replace it? because preg_replace does not do anything if it does not match anything.

yannickl88 · 2017-10-31T11:40:15Z

src/EventListener/AngularHtmlListener.php

@@ -51,7 +51,7 @@ public function onPostTranspile(AssetEvent $event): void
        $content = preg_replace_callback(
            '/templateUrl\s*:(\s*[\'"`](.*?)[\'"`]\s*)/m',
            function ($match) use ($file, $reader) {
-                return 'template: ' . json_encode($this->getCompiledAssetFor($match[2], $file, $reader));
+                return 'template: ' . json_encode($this->getCompiledAssetFor('./' . $match[2], $file, $reader));


what if these are absolute paths? Or is that not possible?

technically it's possible, but in general it doesn't happen, but I can add it if you want....

nicoschoenmaker

There are a lot of adjustments that still need unit-tests. Otherwise these fixes might break in the future.

nicoschoenmaker · 2017-10-31T12:21:17Z

src/Config/ConfigInterface.php

@@ -61,7 +61,6 @@ public function getPlugins(): array;
    /**
     * Return the output folder in which to dump the compiled assets.
     *
-     * @param bool $include_public_folder Whether to include the web/ directory


Not sure why this doc is removed?

merge mistake....

nicoschoenmaker · 2017-10-31T12:23:16Z

src/Bundler/Runner/js/tsc.js

            target: ts.ScriptTarget.ES5,
            module: ts.ModuleKind.CommonJS,
            moduleResolution: ts.ModuleResolutionKind.NodeJs,
+            emitDecoratorMetadata: true,


Why would we enable this?

The docs of typescript point to this issue: microsoft/TypeScript#2577

But that does not tell me anything about why this is (or isn't) a good idea.

Angular require this to be turned on.

nicoschoenmaker · 2017-10-31T12:24:04Z

src/Config/FileConfig.php

@@ -40,10 +40,14 @@ public function __construct(
        array $plugins = [],
        bool $dev = false,
        EventDispatcherInterface $dispatcher = null,
-        LoggerInterface $logger = null
+        LoggerInterface $logger = null,
+        array $non_static_options = []


This added argument is not documented in the docblock.

nicoschoenmaker · 2017-10-31T12:26:52Z

src/EventListener/AngularHtmlListener.php

-        if ($file_path[0] === '.' && $owning_file->dir !== '.' && !empty($owning_file->dir)) {
-            $file_path = $owning_file->dir . '/' . $file_path;
-        }
+        $file_path = preg_replace('/\.css$/', '.less', $owning_file->dir . '/' . $file_path);


This method is called for both style and for the template, so hardcoding anything with .css or .less doesn't feel right.

nicoschoenmaker · 2017-10-31T12:31:28Z

src/Resources/require.js

        }

        _modules[name] = {
            _initializer: initializer,
-            _module: null
+            _module: null,
+            _stack: (new TypeError).stack


What is the impact on this for client side performance? I understand it's nice for debugging, but wouldn't this mean creating >100 objects for a javascript intensive app?

nicoschoenmaker · 2017-10-31T12:32:04Z

test/Config/FileConfigTest.php

@@ -28,7 +28,6 @@ public function testMinimal()
        self::assertSame([], $config->getEntryPoints());
        self::assertSame([], $config->getAssetFiles());
        self::assertSame(self::EXPECTED_OUTPUT, $config->getOutputFolder());
-        self::assertSame('dev', $config->getOutputFolder(false));


Why are these test-cases removed?

nicoschoenmaker · 2017-11-01T09:06:51Z

src/Bundler/Pipeline/ContentPipeline.php

@@ -94,7 +94,7 @@ public function push(array $dependencies, ReaderInterface $file_reader, File $ta
            $module_name = $file->getName();

            if (!empty($this->config->getSourceRoot())
-                && false !== strpos($module_name, $this->config->getSourceRoot())
+                && 0 === strpos($module_name, $this->config->getSourceRoot())


This is a fix for probably a bug that exists for which no unit-test has been added.

nicoschoenmaker · 2017-11-02T08:16:11Z

src/Bundler/Processor/ModuleProcessor.php

@@ -26,7 +26,9 @@ public function peek(string $cwd, ContentState $state): void

    public function transpile(string $cwd, ContentItem $item): void
    {
-        $js  = "register('" . $item->module_name . "', function (define, require, module, exports) {\n";
+        $js  = "register("
+            . json_encode($item->module_name, JSON_UNESCAPED_SLASHES)


This is a fix for probably a bug that exists for which no unit-test has been added.

this is just a common XSS fix

nicoschoenmaker · 2017-11-02T08:18:37Z

src/Config/FileConfig.php

     */
    public function __construct(
        string $config_file,
        array $plugins = [],
        bool $dev = false,
        EventDispatcherInterface $dispatcher = null,
-        LoggerInterface $logger = null
+        LoggerInterface $logger = null,
+        array $non_static_options = []


What is the use case for these non static options? Especially the docblock type mixed scares me.

Maybe we should remove the FileConfig and move the ArrayConfig from the bundle here instead. Or make the FileConfig class a wrapper around the ArrayConfig class.

I had the issue I have one setting which is not statically determined. I can generate a json file and use this class or indeed make a wrapper class for just one setting or I could just add this argument to keep it simple

seems to be only used in the test? There is an interface, just mock it?

nicoschoenmaker · 2017-11-02T08:22:10Z

src/Plugin/LessPlugin.php

+        $plugin_api->getConfig()->getEventDispatcher()->addListener(
+            AssetEvents::FIND_SOURCE,
+            function (FindSourceEvent $ev) {
+                $file_path = preg_replace('/\.css$/', '.less', $ev->getFilepath());


Why wouldn't the Angular component refer to the .less file immediately? That would also help the IDE to find the file.

if Angular Ahead of Time would be used, using .less inmediately would load the less file in as css.

nicoschoenmaker · 2017-11-02T08:22:55Z

src/Import/BuiltIn/AngularImportCollector.php


 /**
 * Angular asset resolver.
 */
 final class AngularImportCollector implements ImportCollectorInterface
 {
+    private $plugin_api;
+
+    public function __construct(PluginApi $plugin_api)


This appears to only need the EventDispatcher.

yannickl88 · 2017-11-02T14:24:40Z

test/Config/FileConfigTest.php

+            true,
+            null,
+            null
+        );


is this change still needed? I think those extra arguments are defaults

nicoschoenmaker · 2017-11-14T15:40:39Z

src/EventListener/AngularHtmlListener.php

-
-        if ($file_path[0] === '.' && $owning_file->dir !== '.' && !empty($owning_file->dir)) {
-            $file_path = $owning_file->dir . '/' . $file_path;
+        if (substr($linked_file, 1, 0) === '/') {


php -r "var_dump(substr('ABC', 1, 0));"

string(0) ""

Based on that, this if can not become true. There should probably be a unit-test for this case.

nicoschoenmaker · 2017-11-14T15:42:46Z

Can we tear this pull request in 10 pieces or so?

This appears to fix a lot of issues at the same time, it's hard to see how these are related now. Or whether all the unit-tests are complete, etc.

pjordaan force-pushed the master branch from 508ed8c to 58dd3d7 Compare October 31, 2017 11:30

yannickl88 reviewed Oct 31, 2017

View reviewed changes

nicoschoenmaker suggested changes Oct 31, 2017

View reviewed changes

pjordaan force-pushed the master branch from 58dd3d7 to a6c2b5f Compare October 31, 2017 14:56

nicoschoenmaker reviewed Nov 1, 2017

View reviewed changes

pjordaan force-pushed the master branch 2 times, most recently from 2f2501f to 40fd71c Compare November 1, 2017 12:42

nicoschoenmaker reviewed Nov 2, 2017

View reviewed changes

pjordaan force-pushed the master branch from 40fd71c to e38028d Compare November 2, 2017 13:55

yannickl88 reviewed Nov 2, 2017

View reviewed changes

pjordaan force-pushed the master branch 5 times, most recently from e794cd8 to de23183 Compare November 9, 2017 10:05

nicoschoenmaker reviewed Nov 14, 2017

View reviewed changes

This was referenced Nov 14, 2017

Support libraries that are already initialized #34

Merged

Protect against odd characters in module name #35

Merged

Module name must start with source root, not simply contain it #36

Merged

pjordaan force-pushed the master branch from de23183 to fd9316c Compare November 22, 2017 08:07

This was referenced Nov 22, 2017

Ignore .d.ts entirely #38

Merged

Only strip .ts from module name #39

Merged

pjordaan force-pushed the master branch from fd9316c to 9a89aff Compare November 22, 2017 12:24

nicoschoenmaker mentioned this pull request Nov 22, 2017

Replaced FileConfig with a in memory config #42

Merged

many fixes

c10c06c

pjordaan force-pushed the master branch from 7a506f5 to c10c06c Compare November 23, 2017 14:26

pjordaan closed this Nov 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many fixes #27

many fixes #27

pjordaan commented Oct 31, 2017

yannickl88 Oct 31, 2017

yannickl88 Oct 31, 2017

pjordaan Oct 31, 2017

nicoschoenmaker left a comment

nicoschoenmaker Oct 31, 2017

pjordaan Oct 31, 2017

nicoschoenmaker Oct 31, 2017

pjordaan Oct 31, 2017

nicoschoenmaker Oct 31, 2017

nicoschoenmaker Oct 31, 2017

nicoschoenmaker Oct 31, 2017

nicoschoenmaker Oct 31, 2017

nicoschoenmaker Nov 1, 2017

nicoschoenmaker Nov 2, 2017

pjordaan Nov 2, 2017

nicoschoenmaker Nov 2, 2017

pjordaan Nov 2, 2017

yannickl88 Nov 2, 2017

nicoschoenmaker Nov 2, 2017

pjordaan Nov 2, 2017

nicoschoenmaker Nov 2, 2017

yannickl88 Nov 2, 2017

nicoschoenmaker Nov 14, 2017

nicoschoenmaker commented Nov 14, 2017

many fixes #27

many fixes #27

Conversation

pjordaan commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoschoenmaker 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

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

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

nicoschoenmaker commented Nov 14, 2017