Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test around define([], function(a, b, c) {}) #14

Closed
thecodedrift opened this issue Mar 20, 2013 · 10 comments
Closed

Add test around define([], function(a, b, c) {}) #14

thecodedrift opened this issue Mar 20, 2013 · 10 comments

Comments

@thecodedrift
Copy link
Contributor

requirejs/requirejs#669

According to the spec, if dependencies are listed as an empty array, this is to be treated differently than if the dependencies argument is omitted completely.

@jrburke
Copy link
Contributor

jrburke commented Mar 20, 2013

I'm open to a test being added for this. I do not have the time at the moment, but feel free to add it yourself. I plan on fixing the issue very soon, for the next requirejs release, and all tests are opt in anyway by implementations.

@thecodedrift
Copy link
Contributor Author

Quick guidance: Since this was in the spec, but a missed test, do we force-opt people in, or do we give it a new test class?

basic_empty?

@jrburke
Copy link
Contributor

jrburke commented Mar 26, 2013

I think any new tests should always be opt-in. The spec is all opt-in. Some specialty loaders do not support the full breadth for implementation size/specific use case goals.

basic_empty_deps?

@thecodedrift
Copy link
Contributor Author

Sounds good. I'll give it a go since I'm in AMD test writing mode.

@unscriptable
Copy link
Member

I disagree. This test case should not be optional. This case and its
behavior has been described in the spec for quite some time. To guarantee
consistency across AMD tools, we need to enforce correct behavior. Don't
we?

On Tue, Mar 26, 2013 at 7:42 PM, Jakob Heuser [email protected]:

Sounds good. I'll give it a go since I'm in AMD test writing mode.


Reply to this email directly or view it on GitHubhttps://github.com//issues/14#issuecomment-15496057
.

@jrburke
Copy link
Contributor

jrburke commented Mar 27, 2013

I have never forced any AMD implementation to pass a particular set of tests, each implementer is responsible for configuring which tests to run for their implementation.

What these tests should do is give someone who wants to decide between an AMD loader, some info on which one they should use. I think it will reflect poorly on an implementation if it cannot pass many tests though. Particularly the "basic" set.

@unscriptable
Copy link
Member

On Wed, Mar 27, 2013 at 3:32 PM, James Burke [email protected]:

I have never forced any AMD implementation to pass a particular set of
tests, each implementer is responsible for configuring which tests to run
for their implementation.

If we're not even enforcing a basic set of tests, then there is no
standard, effectively.

What these tests should do is give someone who wants to decide between an
AMD loader, some info on which one they should use. I think it will reflect
poorly on an implementation if it cannot pass many tests though.
Particularly the "basic" set.

These tests are for implementors. End users don't even look at these
tests. Most don't know they exist. If end users try a few AMD tools and
get inconsistent results, they'll look for other solutions.

-- John


Reply to this email directly or view it on GitHubhttps://github.com//issues/14#issuecomment-15548001
.

@jrburke
Copy link
Contributor

jrburke commented Mar 27, 2013

@unscriptable For me, the main point is that we have tests that can be run. It is not like we go around policing libraries that claim they are AMD, or have any official certification process. I certainly do not have the time for that. I think the market eventually weeds them out.

I also do not want to hold off merging in tests to wait for implementers to make sure they are conformant. It is harder for the implementer to pull a branch of a pull request, test and then merge in. To me, it has always been good to merge the tests to master first, and tell implementers to then add it if they want, and if there is a severe problem with one of the tests, then we fix it and merge back in and repeat. I also do not want good tests that people could use now to wait in some queue while the slowest person in the group finally gets to testing.

I do not think we are disagreeing on core principles, just on the specific ordering of steps and number of steps to get to that end. I think how it has been done to date requires the least amount of work. If you want to suggest a different way and are going to commit the time to ensure the new way is consistently followed going forward, I am fine with that. Best to discuss on the list though.

@thecodedrift
Copy link
Contributor Author

@jrburke @unscriptable I think the biggest difference here is that these tests address something in the spec we were never testing (versus new functionality).

If we want this to be opt-in, we should reword the spec emphasis mine:

The dependencies argument is optional. If omitted, it should may default to ["require", "exports", "module"]. However, if the factory function's arity (length property) is less than 3, then the loader may choose to only call the factory with the number of arguments corresponding to the function's arity or length.

I'll pick up this thread and put it on the list (if nobody else does) once I'm out of my last meeting for the day and not typing & running.

@jrburke
Copy link
Contributor

jrburke commented Mar 27, 2013

OK, if this is just about this particular test, then fine, auto add it to all the implementations. We're spending way too much time on this.

thecodedrift added a commit that referenced this issue Mar 28, 2013
Spec test update for define without dependencies, correction for anon_relative

Fixes #13 #14 #15 #16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants