[DNM] Superseceded by #16

dbouwman commented 9 years ago

I'm not using all the options of this addon, so I can only really test a subset of scenarios. So - if someone could verify that this works if you set loader to requirejs or dojo that would be great.

Also - while this code works, it's far from elegant - feedback is very welcome.

Added

support for inlining of scripts & updated README w/ this info
Changed
if not inlined, the amd-start and amd-config scripts are fingerprinted to enable cache-busting
also ensured that other script tags in the body are not removed (i.e. google analytics)
removed debugging cruft from start-templates.txt

dbouwman commented 9 years ago

Ok - this is rebased on master.

ffaubry commented 8 years ago

@dbouwman I'm still reviewing the code. But as a general discussion for you and @odoe, should we really give the option to inline or not inline? I think that your proposal is good and I would personally simplify the configuration by only supporting inline.

ffaubry commented 8 years ago

@dbouwman This is a great first stab at the issue. I think there is room for simplifications. Let me know if you want me to fork and propose improvements.

dbouwman commented 8 years ago

@ffaubry agreed on all the things you point out - I'll take another stab at this tonight - the first round was definitely "make it work", now I'll try to make it (more) elegant :)

dbouwman commented 8 years ago

@ffaubry @odoe In the near future we will be adding a node app that will be serving the index.html with some "site-specific" payloads inlines (feature flags and json that configures the header/footer and homepage content).

In that scenario, I do want to have the option to have these files on the CDN, so I will keep inlining as an option, but - since we are likely an edge case, I will default to inlining.

dbouwman commented 8 years ago

@ffaubry I think I've addressed your concerns, as well as streamlined things a bit more. Pls take a peek and let me know.

jrowlingson commented 8 years ago

@dbouwman if inline: true option is set and the fingerprint.enabled option is set to false we do not want to fingerprint the assets/amd-*.js files.

dbouwman commented 8 years ago

@jrowlingson fixed.

@ffaubry / @odoe can we get this merged and a new npm release pushed?

jrowlingson commented 8 years ago

@dbouwman Thanks!

@ffaubry / @odoe This is perhaps unrelated but we are seeing this following error on live reloads for development (local) builds:

2015-12-09 10:30:20.756 Navigated to http://localhost:4200/projects
2015-12-09 10:30:21.776 init.js:31 Error: defineAlreadyDefined(…)(anonymous function) @ init.js:31(anonymous function) @ init.js:9a @ init.js:5r.signal @ init.js:9async @ init.js:32(anonymous function) @ init.js:32
2015-12-09 10:30:21.782 init.js:31 src: dojoLoader
2015-12-09 10:30:21.782 init.js:31 info: 0
2015-12-09 10:30:21.782 init.js:32 .
2015-12-09 10:30:22.395 init.js:31 Error: multipleDefine(…)(anonymous function) @ init.js:31(anonymous function) @ init.js:9a @ init.js:5r.signal @ init.js:9Ja @ init.js:27Ia @ init.js:31(anonymous function) @ init.js:1838
2015-12-09 10:30:22.396 init.js:31 src: dojoLoader
2015-12-09 10:30:22.396 init.js:31 info: Object {pid: "esri", mid: "esri/jsapi", pack: Object, url: "http://js.arcgis.com/3.14/esri/jsapi.js", executed: 0…}cjs: Objectdef: ()deps: Array[27]executed: 0injected: 2isAmd: trueisXd: truemid: "esri/jsapi"pack: Objectlocation: "../esri"main: "main"name: "esri"__proto__: Objectpid: "esri"result: Object__proto__: Objecturl: "http://js.arcgis.com/3.14/esri/jsapi.js"__proto__: Object
2015-12-09 10:30:22.396 init.js:32 .

ffaubry commented 8 years ago

@jrowlingson Are you using the latest version of the addon?

dbouwman commented 8 years ago

fwiw @jrowlingson we are not seeing that in our app

jrowlingson commented 8 years ago

@ffaubry we building against this PR

ffaubry commented 8 years ago

@dbouwman This PR is not yet ready to be merged. What jack is experiencing is when ember build --watch is used or ember serve is used, the output files are not recreated correctly. This is why in the original code, I'm using sha to detect if the index has to be rebuilt or not. I'm currently trying to solve this issue on a different fork, it's too difficult for me to rework your code. We will see what I get.

dbouwman commented 8 years ago

@ffaubry yeah - very odd, b/c I was working using ember s all day, with no issues. But then we don't change much that impacts the amd imports etc, so it's very possible that I'm just not hitting the same flows you are.

ffaubry commented 8 years ago

@dbouwman I tried to use you PR in our project and ran into many issues during rebuild. Some poited out by @jrowlingson. I tried to initially fix your PR but it was way to complicated. I ended up writing my own PR. I got greatly inspired by you PR. Please test https://github.com/Esri/ember-cli-amd/pull/16. If this is working for you we will go with that one.

The main differences are:

I don't rebuild the index if nothing has changed
if the list of modules is different then the index and optionally the start scripts will be refreshed
The config script is common to the test and the app index files. So it should be read and saved only once
I made sure that if the scripts are not inlined then they will be fingerprinted if requested

dbouwman commented 8 years ago

@ffaubry Awesome & Thanks for diving into it! Given my limited history working with AMD in general, and zero background in the guts of the Ember build process, I'm actually quite surprised I got this branch to almost work. I'm gonna pull your branch and give it a quick test run :)

Esri / ember-cli-amd

[DNM] Superseceded by #16 #14

Added

Changed