search

OpenF2 / F2

Redefining web integration for the financial services community
Apache License 2.0
130 stars 62 forks source link

Pull Request for "AMD Plugin" branch #125

Closed montlebalm closed 4 years ago

montlebalm commented 11 years ago

I've added an "F2App" plugin for RequireJS that will allow containers to load F2 apps more easily.

I have also made a handful of bug fixes including:

The expected behavior from my changes has been encapsulated in a series of tests in "/tests/spec/amd-spec.js".

markhealey commented 11 years ago

@montlebalm I'd like to merge this into a 1.2.3-wip branch (which hasn't been created yet) instead of master since this is likely to be launched with some other updates. Keep working in amd-plugin and when your documentation changes are done let's close this and open a new pull request for 1.2.3-wip <- amd-plugin. Thanks.

markhealey commented 11 years ago

After discussions with @ilinkuo and @rwadkins yesterday, I think we should hold off on a pull request and iterate in this amd-plugin branch until we get it right.

ilinkuo commented 11 years ago

@markhealey, @montlebalm Thanks for holding off on this. I'm going to go over this over this weekend to see if there's anything we'd like to change. I think that in the future, for a major feature branch like this, it would be good to announce it on the google groups, or on a F2Dev twitter channel. For example, something like 

Chris is starting work on introducing AMD into F2. Please check out the branch <link> and offer your feedback!

twitted or announced via Google Groups on 8/22, the day that Chris started, would have been good. This singles out this event as something worth noting over all the other Github events.

ilinkuo commented 11 years ago

Useful trick -- ignore whitespace in github diffs by adding ?w=1. See https://github.com/blog/967-github-secrets. You won't be able to comment in the ignore whitespace view. however. You have to go back to the original diff to make comments.

markhealey commented 11 years ago

PS - I didn't realize GitHub.com copied comments made on a specific commit into the pull request tracking those commits. This site never ceases to impress me—collaboration at its best.

markhealey commented 11 years ago

@ilinkuo, @brianbaker, @Raylehnhoff, @montlebalm - I have opened #129 per my comment.

rwadkins commented 11 years ago

I'm trying to get up to speed on this conversation, but I'm missing a bit of background and a failure to understand the real problem statement. @ilinkuo, is the crux of your issue with this solution is that it's too far removed from an AMD solution to be considered useful in an AMD environment? Is it possible to highlight the feature gaps of the current loader, this solution, and a "true" AMD solution? Then consider what would be missing in an AMD solution that the current loader provides?

I hate it when latecomers slow down a conversation ;)

markhealey commented 11 years ago

Not at all, @rwadkins. Good thoughts.

ilinkuo commented 11 years ago

@rwadkins

I'll summarize my position. Let me know if you have any other questions

The pull request under discussion provides an easier way to load F2 Apps by implementing a RequireJS/AMD plugin. However, it is my opinion that

Thus, it would be better to leave AMD out of this entirely and rewrite the functionality as a new method F2.easyLoad() which could be useful to both AMD and non-AMD users.

In the course of discussion, @montlebalm proposed an alternate form of the proposed plugin API. Both @markhealey and I preferred this form. I recommended that this alternate form be used, ASAP. However, as there is no code yet written for the alternate form, MOD would like to move forward by modifying the existing pull request, though they haven't said whether they intend to change the API. If the revised pull request still uses the original proposed API, then I am opposed, because there is no way to smoothly iterate from one API to the other.