Esri / ember-cli-amd

Ember CLI Addon for using AMD libraries
Apache License 2.0
21 stars 15 forks source link

Add-on Update. Allow AMD files to not be inlined #46

Closed ggayowsky closed 4 years ago

ggayowsky commented 5 years ago

The PR aims to update this add-on in several ways. Fixed some minor bugs along the way

ffaubry commented 5 years ago

@ggayowsky I see that your PR is multi dimensional...

On one side, I really like that you are cleaning the code and introducing tests. The new feature makes sense as well. However, you moved code around and introduced new features at the same time. This is making the code review really hard. I need to know what you changed in the code you moved. I will take the time to review it in details. This code is used by ArcGIS Dashboard, I need to be extra cautious (I know tests would have been great).

In addition, I will go ahead and merge https://github.com/Esri/ember-cli-amd/pull/47. This will have an impact on your PR.

ggayowsky commented 5 years ago

I wrote this a while a go, but I will try to remember exactly what code I changed.

The lib/replace-require-and-define.js basically the old Require filter. It just uses ES6 class definition instead of the modifying the prototype.

At it's core, it replaces the require and define with a non-conflicting name for each file changed.

The biggest change is now that on each rebuild, the replace-require-and-define filter rebuilds the list of amdModules used in the project (and thus updates index.html) whenever an amd modules is added or removed (that is what the build and concatModulesCache functions are doing)

The biggest change is the IndexHTMLWriter Plugin. Before, the scripts use to modify the index.html in the postBuild step of the cli, this meant that any changes to the index.html would not go through any other steps (such as fingerprinting). This is based off of the old indexBuilder function, and should following the same flow, however, I needed to add the ability to not inline scripts (as it was failing unsafe-inline CSP directive). That is where the majority of the difference comes from.

The other big difference is the way to add-on goes registers the plugins / features.

The postprocessTree step now:

  1. Runs the replaceRequireAndDefineFilter on all js files (needed to exclude the html files for mergeTree purposes)
  2. Watches the html files and the specified config file to ensure that the IndexHTMLWriter is run when either index.html or the specified config file has changed.
  3. Finally merges those two trees to ensure that when any of the JS, HTML or amd config file changes the add-on is run.

The lib/start-template.txt is the old start-template.txt

A good way to test that thing still work (with these changes) on your build is to temporarily change your package.json to point ember-cli-amd to my repo on the feature/allow-injected-javascript-in-src-files branch. (https://github.com/ggayowsky/ember-cli-amd/tree/feature/allow-injected-javascript-in-src-files)

Let me know if you have any more questions.

ffaubry commented 5 years ago

@ggayowsky I have updated master with some of your ideas. You should refresh this PR. It will only show the 'do-not-inline-config' part that you are after. This way we will have a better view of what you are updating. Thx for initiating this update.

ffaubry commented 4 years ago

The release 3.0.0 is solving this issue. Closing this PR.