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

Hide jQuery.camelCase #3604

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@niltoncsr
Contributor

niltoncsr commented Apr 2, 2017

Here, I try to resolve this issue by moving the exposed method jQuery.camelCase() into a internal module camelCase located in src/core/camelCase.js, as well as importing it wherever it's been used.

Core: make camelCase function available only for internal usage
Move camelCase function from jQuery to an exclusive module and inject it
wherever is needed, modifying only where it is called.

Fixes gh-3384
@jsf-clabot

This comment has been minimized.

Show comment
Hide comment
@jsf-clabot

jsf-clabot Apr 2, 2017

CLA assistant check
All committers have signed the CLA.

jsf-clabot commented Apr 2, 2017

CLA assistant check
All committers have signed the CLA.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 2, 2017

@niltoncsr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @mgol and @davids549 to be potential reviewers.

mention-bot commented Apr 2, 2017

@niltoncsr, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @mgol and @davids549 to be potential reviewers.

@niltoncsr

This comment has been minimized.

Show comment
Hide comment
@niltoncsr

niltoncsr Apr 2, 2017

Contributor

I need some help with the test cases.
What is the best practice, in this scenario, to import an AMD module into the unit test? After code review, I'll commit the unit tests too.

Contributor

niltoncsr commented Apr 2, 2017

I need some help with the test cases.
What is the best practice, in this scenario, to import an AMD module into the unit test? After code review, I'll commit the unit tests too.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 2, 2017

Member

@niltoncsr We don't use those tiny AMD modules in tests. In this case, there are two things to do:

  1. jQuery.camelCase tests should be moved to test/unit/deprecated.js.
  2. Any tests that don't test jQuery.camelCase but use it inside of the test should be changed to not rely on that (if there are any such tests, I haven't checked).

Let me know if you have any further questions. And thanks for the PR!

Member

mgol commented Apr 2, 2017

@niltoncsr We don't use those tiny AMD modules in tests. In this case, there are two things to do:

  1. jQuery.camelCase tests should be moved to test/unit/deprecated.js.
  2. Any tests that don't test jQuery.camelCase but use it inside of the test should be changed to not rely on that (if there are any such tests, I haven't checked).

Let me know if you have any further questions. And thanks for the PR!

@niltoncsr

This comment has been minimized.

Show comment
Hide comment
@niltoncsr

niltoncsr Apr 2, 2017

Contributor

Thanks for the feedback, @mgol! 😄

I got it, I have to remove all tests that use jQuery.camelCase, but I still have doubts on how to test the new module: its importing and usage. What are your thoughts on that? Where can I proceed for testing this module?

Contributor

niltoncsr commented Apr 2, 2017

Thanks for the feedback, @mgol! 😄

I got it, I have to remove all tests that use jQuery.camelCase, but I still have doubts on how to test the new module: its importing and usage. What are your thoughts on that? Where can I proceed for testing this module?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 2, 2017

Member

I have to remove all tests that use jQuery.camelCase

Not remove but move. :)

but I still have doubts on how to test the new module: its importing and usage

You don't have to test the camelCase module. Since it's now internal and only temporarily exposed as a deprecated jQuery.camelCase, all we care (with the exception of temporarily testing jQuery.camelCase in test/unit/deprecated.js) is that public API output is what we expect it to be.

If we broke jQuery.camelCase, many tests in the data & css modules should fail. It'd be a good idea to try that - e.g. change the function to not camelCase and see if tests are failing in css & data modules. If not, some tests have to be added for them. You can try doing that to be sure.

Member

mgol commented Apr 2, 2017

I have to remove all tests that use jQuery.camelCase

Not remove but move. :)

but I still have doubts on how to test the new module: its importing and usage

You don't have to test the camelCase module. Since it's now internal and only temporarily exposed as a deprecated jQuery.camelCase, all we care (with the exception of temporarily testing jQuery.camelCase in test/unit/deprecated.js) is that public API output is what we expect it to be.

If we broke jQuery.camelCase, many tests in the data & css modules should fail. It'd be a good idea to try that - e.g. change the function to not camelCase and see if tests are failing in css & data modules. If not, some tests have to be added for them. You can try doing that to be sure.

Tests: move unit tests mentioning the deprecated jQuery.camelCase
Move its single exclusive unit test from the core testing module to the
deprecated, and adapt other unit tests that inside makes use of jQuery.camelCase
to not rely on it.

Fixes gh-3384
Closes gh-3604
@niltoncsr

This comment has been minimized.

Show comment
Hide comment
@niltoncsr

niltoncsr Apr 3, 2017

Contributor

I made that now, @mgol. Moved the single unit test to test/unit/deprecated.js (while there it still doesn't pass) and removed any calls to jQuery.camelCase, adapting the unit tests' labels and expectations asserts.
Is there anything I missed?

Moving the conversation, is there any intention or planning to test an internal usage module? Despite its a internal module, like in this case, it should be test since other features and methods rely on it. I say that we must something about it. What are the your thoughts on that subject?

Contributor

niltoncsr commented Apr 3, 2017

I made that now, @mgol. Moved the single unit test to test/unit/deprecated.js (while there it still doesn't pass) and removed any calls to jQuery.camelCase, adapting the unit tests' labels and expectations asserts.
Is there anything I missed?

Moving the conversation, is there any intention or planning to test an internal usage module? Despite its a internal module, like in this case, it should be test since other features and methods rely on it. I say that we must something about it. What are the your thoughts on that subject?

@@ -625,7 +625,6 @@ QUnit.test( ".data always sets data with the camelCased key (gh-2257)", function
div.data( key, val );
var allData = div.data();
assert.equal( allData[ key ], undefined, ".data does not store with hyphenated keys" );
assert.equal( allData[ jQuery.camelCase( key ) ], val, ".data stores the camelCased key" );

This comment has been minimized.

@mgol

mgol Apr 3, 2017

Member

This assertion is very important and shouldn't be removed. Instead, I'd expect the datas object to be modified to contain the expected camelCased value, sth like:

datas = {
	"non-empty": {camelCasedKey: "nonEmpty", value: "a string"},
	...
};

The same applies to other places where you removed the assertions using jQuery.camelCase. It's not the assertions that need removal but jQuery.camelCase usage.

@mgol

mgol Apr 3, 2017

Member

This assertion is very important and shouldn't be removed. Instead, I'd expect the datas object to be modified to contain the expected camelCased value, sth like:

datas = {
	"non-empty": {camelCasedKey: "nonEmpty", value: "a string"},
	...
};

The same applies to other places where you removed the assertions using jQuery.camelCase. It's not the assertions that need removal but jQuery.camelCase usage.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 3, 2017

Member

Moved the single unit test to test/unit/deprecated.js (while there it still doesn't pass)

What do you mean by "while there it still doesn't pass"?

Moving the conversation, is there any intention or planning to test an internal usage module? Despite its a internal module, like in this case, it should be test since other features and methods rely on it.

Not necessarily. While testing internal APIs is a valid approach, it's not the only one. This is a private module, just for jQuery internal purposes. Therefore, if any changes land to it that change its behavior but that don't affect any public API in any visible way, that doesn't matter. We should make sure all public methods are tested well enough that testing internal modules is not necessary.

Member

mgol commented Apr 3, 2017

Moved the single unit test to test/unit/deprecated.js (while there it still doesn't pass)

What do you mean by "while there it still doesn't pass"?

Moving the conversation, is there any intention or planning to test an internal usage module? Despite its a internal module, like in this case, it should be test since other features and methods rely on it.

Not necessarily. While testing internal APIs is a valid approach, it's not the only one. This is a private module, just for jQuery internal purposes. Therefore, if any changes land to it that change its behavior but that don't affect any public API in any visible way, that doesn't matter. We should make sure all public methods are tested well enough that testing internal modules is not necessary.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 3, 2017

Member

One more thing - we're only deprecating the method for now, not removing it. Therefore, it should be defined in src/deprecated.js as an alias to the internal camelCase. Once you do that, all tests should pass.

Member

mgol commented Apr 3, 2017

One more thing - we're only deprecating the method for now, not removing it. Therefore, it should be defined in src/deprecated.js as an alias to the internal camelCase. Once you do that, all tests should pass.

@niltoncsr

This comment has been minimized.

Show comment
Hide comment
@niltoncsr

niltoncsr Apr 3, 2017

Contributor

Oh, now I got what you mean 😄
So, creating an alias on src/deprecated.js removes the feature from core but still expose it, as deprecated.

Just one more question:
Should I still change the datas object to make the reintroduced assertions pass or since

Once you do that, all tests should pass.

should I just rollback them as they were before? I think the first one, change the datas, it's correct for future purposes of removal. Am I right?

Thanks for your patience, I'm still getting into jQuery's internals. 😅

Contributor

niltoncsr commented Apr 3, 2017

Oh, now I got what you mean 😄
So, creating an alias on src/deprecated.js removes the feature from core but still expose it, as deprecated.

Just one more question:
Should I still change the datas object to make the reintroduced assertions pass or since

Once you do that, all tests should pass.

should I just rollback them as they were before? I think the first one, change the datas, it's correct for future purposes of removal. Am I right?

Thanks for your patience, I'm still getting into jQuery's internals. 😅

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 3, 2017

Member

should I just rollback them as they were before?

If you did that all the tests would pass but when deprecating thing we try to avoid any mentions of them outside of src/deprecated.js and test/unit/deprecated.js. Thanks to that approach, once we decide on removal we simply remove the API from src/deprecated.js and its tests from test/unit/deprecated.js and we're done. So the proper solution is neither to remove those assertions as currently done in this PR nor to keep using the deprecated API there but to reformulate the assertions in a way that they check what they're supposed to check without using deprecated APIs.

Something like what I proposed in #3604 (comment) should work.

Let me know if anything is still unclear. Thanks!

Member

mgol commented Apr 3, 2017

should I just rollback them as they were before?

If you did that all the tests would pass but when deprecating thing we try to avoid any mentions of them outside of src/deprecated.js and test/unit/deprecated.js. Thanks to that approach, once we decide on removal we simply remove the API from src/deprecated.js and its tests from test/unit/deprecated.js and we're done. So the proper solution is neither to remove those assertions as currently done in this PR nor to keep using the deprecated API there but to reformulate the assertions in a way that they check what they're supposed to check without using deprecated APIs.

Something like what I proposed in #3604 (comment) should work.

Let me know if anything is still unclear. Thanks!

@mgol mgol self-assigned this Apr 3, 2017

@niltoncsr

This comment has been minimized.

Show comment
Hide comment
@niltoncsr

niltoncsr Apr 3, 2017

Contributor

Thanks a lot for the explanation.
I'll make it as you suggested 😃

Contributor

niltoncsr commented Apr 3, 2017

Thanks a lot for the explanation.
I'll make it as you suggested 😃

@timmywil timmywil modified the milestone: 3.3.0 Apr 24, 2017

@timmywil timmywil assigned timmywil and unassigned mgol Jul 31, 2017

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 26, 2017

Member

@niltoncsr Are you still interested in finishing this PR?

Member

mgol commented Nov 26, 2017

@niltoncsr Are you still interested in finishing this PR?

@mgol mgol referenced this pull request Nov 26, 2017

Closed

Deprecate jQuery.camelCase #3384

timmywil added a commit to timmywil/jquery that referenced this pull request Dec 4, 2017

@timmywil timmywil referenced this pull request Dec 4, 2017

Closed

Fixes #3384: deprecate jQuery.camelCase #3874

4 of 4 tasks complete
@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 4, 2017

Member

Closing in favor of #3874. Thank you for your contribution.

Member

timmywil commented Dec 4, 2017

Closing in favor of #3874. Thank you for your contribution.

@timmywil timmywil closed this Dec 4, 2017

timmywil added a commit that referenced this pull request Jan 8, 2018

immpo added a commit to immpo/jquery that referenced this pull request Feb 3, 2018

up (#1)
* Dimensions: ignore transforms when retrieving width/height

Close gh-3561
Fixes gh-3193

* CSS: remove dead code in getWidthOrHeight

- getCSS already falls back to inline styles

Ref gh-3561

* Release: update release dependencies

* Release: update AUTHORS.txt

* Release: update version to 3.2.0-pre

* Release: md5sum -> md5 -r for MAC

* Build: Updating the master version to 3.2.1-pre.

* Release: edit dist README version on release

Fixes gh-3574

* Build: update PR template

- Comment out things we don't need to see in the PR description
- Change CLA link

* Tests: move readywait to an iframe test

Close gh-3576
Fixes gh-3573

* Dimensions: fall back to offsetWidth/Height for inline elems

Close gh-3577
Fixes gh-3571

* Revert "Event: Trigger checkbox and radio click events identically"

This reverts commit b442aba.

* Revert "Event: Add radio click triggering tests"

This reverts commit 5f35b5b.

* Tests: add test for passing trigger data to radio click handler

Close gh-3581
Fixes gh-3579

* Build: Updating the master version to 3.2.2-pre.

* CSS: retrieve inline style before computed

- Fixes an issue with getting computed style on detached elements

* Revert "Build: Updating the master version to 3.2.2-pre."

This reverts commit 066bd86.

* Build: Updating the master version to 3.2.2-pre.

* Tests: Fix incorrect assert name for ensure_iterability_es6

Closes gh-3584
Ref bb026fc.

* Docs: Update links to HTML spec for stripAndCollapse (#3594)

* Offset: Use correct offset parents; include all border/scroll values

Thanks @anseki

Fixes gh-3080
Fixes gh-3107
Closes gh-3096
Closes gh-3487

* Core: Update isFunction to handle unusual-@@toStringTag input

Ref gh-3597
Fixes gh-3600
Fixes gh-3596
Closes gh-3617

* Tests: Improve offset test setup and labels

Hopefully this fixes iOS testing: http://swarm.jquery.org/job/5226

Ref 1d2df77
Closes gh-3641

* Tests: Be even more async for iOS

Ref 1d2df77
Closes gh-3643

* Tests: Attach test iframes to the body for visibility-dependent code

Ref 1d2df77
Closes gh-3645

* Tests: Allow a mock QUnit.test for perfect testIframe fidelity

Ref 1d2df77
Closes gh-3647

* Tests: Prepend test iframes for even *more* consistency

Ref 1d2df77

* Tests: Reset iframe window scroll after updating html/document position

Ref 1d2df77
Closes gh-3649

* Tests: Add debugging to investigate iOS failures

Ref 1d2df77

* Tests: Keep iframes visible in TestSwarm

Ref 1d2df77

* Tests: Adjust by actual scroll position, rather than expected

Ref 1d2df77

* Tests: Clean up offset debugging

Ref 1d2df77
Ref c0edd8d

* Tests: Correct expected assertion count

Ref e94b5b0

* Tests: Revert some testIframe changes to fix dimensions tests

Ref c0edd8d

* Revert "Tests: Revert some testIframe changes to fix dimensions tests"

This reverts commit c4368a9.

* Tests: Revert some testIframe changes to fix dimensions tests

Ref c0edd8d

* CSS: Drop the float mapping from cssProps

Firefox 35 and newer support style.float directly.

Closes #3569

* Docs:Tests: Update IE/Edge-related support comments & tests

Closes gh-3661

* Build: Test on Node.js 8, stop testing on Node.js 7

* Tests: minor typos

Close gh-3671

* Dimensions: Include scroll gutter in "padding" box

Fixes gh-3589
Closes gh-3656

* Deferred: fix memory leak of promise callbacks

Fixes gh-3606
Closes gh-3657

* Build: update node dependencies; commit package-lock.json

- Also ignore yarn.lock
Close gh-3669

* Build: Update sinon, husky, and qunitjs

* Build: fix uglify options for uglify update

- Uses new typeofs option for compression
- See mishoo/UglifyJS2#2198

Close gh-3710

* Event: `stopPropagation()` on native event-handler

Fixes gh-3693
Close gh-3694

* Core: Deprecate jQuery.isWindow

Fixes gh-3629
Close gh-3702

* Test: ensure position/offset return mutable objects

Fixes gh-3612
Closes gh-3695

* Revert "Offset: Resolve strict mode ClientRect "no setter" exception"

This reverts commit 3befe59.

* Offset: fix error from bad merge in #3695

* Dimensions: Detect and account for content-box dimension mishandling

Fixes gh-3699
Closes gh-3700

* Support: Properly check for IE9 absolute scrollbox mishandling

Ref gh-3589
Fixes gh-3699
Fixes gh-3730
Closes gh-3729

* Tests: Try extra hard to control focus

Ref gh-3732

* Tests: Abort focus tests when the environment doesn't cooperate

Ref gh-3732

* Tests: Reduce the abort timeout for simple focus testing

Ref gh-3732

* Tests: Simulate events when CI hinders use of native ones

Ref gh-3732

* Tests: Account for TestSwarm focus issues

Closes gh-3732

* Ajax: add an ontimeout handler to all requests

Fixes gh-3586
Close gh-3590

* Dimensions: Improve offsetWidth/offsetHeight fallback

Fixes gh-3698
Fixes gh-3602
Closes gh-3738

* Tests: Replace non-ASCII whitespace in source code

* Dimensions: Don't trust non-pixel computed width/height

Fixes gh-3611
Closes gh-3741

* Build: Fix comment typo

Closes gh-3747

* Build: Update my name in .mailmap

I got married! 🎉

* Build: Update my name in ATHORS.txt

I forgot .mailmap isn't everything.

* Tests: Update path calculation

Fixes gh-3756
Closes gh-3757

* CSS: Avoid unit-conversion interference from CSS upper bounds

Fixes gh-2144
Closes gh-3745

* Tests: Update lineHeight adjustments to give Android more slop

* CSS: Detect more WebKit styles erroneously reported as percentages

Ref 692f9d4
Fixes gh-3777
Closes gh-3778

* Build: Update to Babel 7, use for-of plugin instead of preset-es2015

Closes gh-3786

* Build: Drop cross-spawn, use child_process.spawn shell option

* Build: increase timeout in Promises/A+ tests 10 times

The promises-aplus-tests sets up a default 200 ms Mocha timeout. This makes
our tests randomly fail on Jenkins. 2 seconds will be safer.

Closes gh-3791

* Build: Remove package-lock.json, add it to .gitignore

npm 5, even the version included in the latest Node.js 8.5.0 re-generates
`package-lock.json` on each install. And when it does on a system that doesn't
support all the optional dependencies that are supported on the OS where the
lockfile was generated, it removes those optional deps from the lockfile.

The effect is that everyone firing `npm install` on our repo on any OS other
than macOS will immediately get a dirty state of the repo as the `fsevents`
dependency subtree gets removed from `package-lock.json`. That's a really bad
experience.

This commit removes package-lock.json from the repository and adds it to
.gitignore. We'll start committing the file again once the issue is resolved
on npm's part.

Fixes gh-3792

* Tests: Make Node tests work for paths with spaces in them

Without this patch Jenkins tests fail as jQuery job names there contain spaces,
e.g. "jQuery Core".

Closes gh-3821

* Tests: Add Safari 11 support test results

* Build: Test on Node.js 9

Closes gh-3840

* Tests: Add iOS 11 support test results

* Manipulation: Reduce size by eliminating single-use variable

Closes gh-3851

* CSS: Correctly set support properties with non-default zoom

Fixes gh-3808
Closes gh-3872

* Docs: Create CODE_OF_CONDUCT.md

Close gh-3865

* Tests: Add support for running unit tests via grunt with karma

- Update QUnit to 1.23.1
- Remove unused dl#dl from test/index.html
- Remove unused map#imgmap from test/index.html
- Ensure all urls to data use baseURI
- Add the 'grunt karma:main' task
  - customContextFile & customDebugFile
- Add 'npm run jenkins' script

Close gh-3744
Fixes gh-1999

* Build: Only run browser tests in one Node version on Travis

Ref gh-3744
Closes gh-3894

* Core: make camelCase function available only for internal usage

Close gh-3604
Fixes gh-3384

* Core: adjust data tests to ensure proper camelCasing

- Add back camelCase to the public object (deprecate not remove)
Ref #3384

* Core: deprecate jQuery.now

Fixes gh-2959
Close gh-3884

* Core: deprecate jQuery.proxy (not slated for removal)

Fixes gh-2958
Close gh-3885

* Manipulation: use `.children` to select tbody elements

- selectors beginning with a child combinator are not valid natively.
  This fixes the tests when using selector-native.js

* Attributes: allow array param in add/remove/toggleClass

+30 bytes instead of +182

Thanks to @faisaliyk for the first pass on this feature.

Fixes gh-3532
Close gh-3917

* Ajax: add unit test for getScript(Object)

Fixes gh-3736
Close gh-3918

* Tests: only run ontimeout test if ontimeout exists

Fixes gh-3742
Close gh-3919

* Build: Fix UglifyJS output in Android 4.0; update uglify

- Thanks to @mgol for first pass

Fixes gh-3743
Close gh-3920

* Tests: fix function reference for unbinding

Ref gh-2958

* Build: Remove CRLF line endings to fix builds on Windows

Close gh-3929

* Core: deprecate jQuery.isFunction

Fixes gh-3609

* Event: Move event aliases to deprecated

Fixes gh-3214

* Ajax: Don't process non-string data property on no-entity-body requests

Fixes gh-3438
Closes gh-3781

* Core: deprecate jQuery.isNumeric

Fixes gh-2960
Closes gh-3888

* Tests: fix weird failure in Edge 16 CSS

Fixes gh-3866
Close gh-3932

* Tests: fix weird flaky attributes test in Edge 16

Fixes gh-3867
Close gh-3931

* Core: deprecate jQuery.type

Fixes gh-3605
Close gh-3895

* Tests: fix number of expected assertions in basic core

* Tests: temporarily require sudo access for karma:main on travis

- This should fix the broken travis build on Node 8
- See travis-ci/travis-ci#8836

* Tests: correctly set sudo in travis config, not karma config

* Manipulation: Add support for scripts with module type

Fixes gh-3871
Close gh-3869

* Tests: fix tests in AMD mode

* Tests: ensure that module assertions run on supported browsers

- Also fixes tests for karma, where the URL for the module is different

Ref gh-3871

* Filter: Use direct filter in winnow

for both simple and complex selectors

Fixes gh-3272
Closes gh-3910

* Build: Add "-debug" suffix to name of karma debug tasks

Ref gh-3922
Close gh-3936

* Tests: skip test with invalid selector for selector-native tests

* Release: add new authors to AUTHORS.txt

* Release: update version to 3.3.0-pre

* Build: Updating the master version to 3.3.1-pre.

* Build: Updating the master version to 3.3.2-pre.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.