Move all JS in modules to a js/ folder

Here's a start.

Yeh I didn't really know what to do with twig_theme_test.js but I've added it for consistency now. Trying to fix more failing tests for now. Likely not yet ready.

Oops, thanks. Missed this one.

Working on the failures. I already found one more path to fix.

That should be it.

Not sure I know what's happening with that last fail.

$this->assertTrue(strpos($rendered_js, 'simpletest.js') < strpos($rendered_js, 'core/misc/tableselect.js'), 'Altering JavaScript weight through the alter hook.');
var_dump(strpos($rendered_js, 'simpletest.js')); // returns int(812)
var_dump(strpos($rendered_js, 'core/misc/tableselect.js')); //returns int(634)
die();

So, the integer returned for each strpos() is not what I was expecting. To make the test pass, we'd have to change the comparison operator to >

Tried to dump the strpos() without this patch and here's what I'm getting:

$this->assertTrue(strpos($rendered_js, 'simpletest.js') < strpos($rendered_js, 'core/misc/tableselect.js'), 'Altering JavaScript weight through the alter hook.');
var_dump(strpos($rendered_js, 'simpletest.js')); // returns int(658)
var_dump(strpos($rendered_js, 'core/misc/tableselect.js')); //returns int(724)
die();

Cancelled test in #16. We should try to test only latest patch.

Just a nit I found with a js file in a test folder. I found that in other modules the js files for tests are also placed in a js folder.

Sorry, forgot to add a file :(

In #6, _nod said:

The last one I can find not in a js subfolder is core/modules/locale/tests/locale_test.js but it's not exactly a module so I say it's fine like that.

Thus why it's been left as is. I guess it's fine to have both approaches so that we can pick the one we eventually prefer.

+1 RTBC

Like all issues hoping to be committed right now, this one needs a beta evaluation. Also, I had to be a nudge but if you mark an issue RTBC it will help the community and the committers if you explain how you came to that conclusion.

I took a crack at a beta evaluation and I don't think this change is unfrozen or prioritized so I suggest postponing it. Please share if you disagree.

@tbradbury I agree. This can be done in the next release.

Drupal tracking All JS via PHP in CORE. And this is the only and must way to use Drupal libs via Drupal API. We should consider these as private and protected scope members. We're doing internal improvement now.

@nod_ and @droplet According to the beta policy the only way this issue can go in now is if it reduces fragility. The decision on fragility is a core maintainer decision (in other words, your decision) and is on a case-by-case basis. This fragility examples from https://www.drupal.org/core/beta-changes do not seem to apply here:

Additionally, certain changes that reduce fragility are also prioritized. For example: Narrowing an API, removing unused and untested functionality, correcting misspelled or wholly inaccurate method names. Whether or not an issue reduces fragility is at core maintainer discretion and should be discussed on a case-by-case basis.

Created reroll.

Patch attached.

8.0.x:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
29

After #44:

$ find core/modules -name '*.js' | grep -v '/js/' | wc -l
0

Looks good.

RTBC as per #45.

Thanks @cilefen and @tbradbury. I agree that this does not really pass the beta evaluation. Even if it is just moving files around internally, there is not any specific reason this change is prioritized during the beta. Unfortunately consistency/best practice/DX are not by themselves enough to justify the change. There isn't really a case for it reducing fragility.

I asked @Wim Leers if this would be reasonable to change in 8.1.x, and he pointed out that since other code can alter these libraries, the paths would be wrong and the alters would break. So there is disruption from this change, and it would need to be postponed to 9.x. It also could break an existing site or module's custom JS for one of these libraries now.

So unfortunately I'm postponing this issue to 9.x. Thanks all for your work on the patch nonetheless. I can see why this would be good to clean up eventually.

We should reconsider once for this changes. D8 is the release with big changes in structures. Let's do it together.

I agree, rerolling against 8.1.x
Fixed conflicts on:

• EngineTwigTest.php
• StatisticsLoggingTest.php
• LocaleJavascriptTranslationTest.php

This should fix the failing test.

Should we add an empty update function to cause a cache rebuild?

@dawehner: that is probably a good idea.

Cool, afaik we add those doc groups for each updates, so we probably want one for 8.1.x

@_nod #54: I think you are wrong. See #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides and #1308152: Add stream wrappers to access extension files will not really solve this if you change subfolders. libraries-override can have paths and this will all break if the folder structure is changed. I'm not sure whether these makes this a D9 issue.

Attempting to address comment #60. Not entirely sure on the docblock, I couldn't find documentation about this on the coding standards, so I just tried to follow what was there previously. Have a look.

This seems a bit problematic for backwards compatibility because of libraries-override and such…

I think I agree with @Cottser - if someone has done the following in their theme...

libraries-override:
  drupal.book:
    js:
      book.js: false

We'd be breaking this no?

Yeah we would be breaking it… I'm a bit torn but overall it feels a bit wrong to potentially break BC for cleanup.

I talked to @catch the other day about this and he mentioned we could maybe do a mapping of old paths to new to preserve the BC. This method could probably also make hook_library_info_alter() done by modules still work which is perhaps a more common use case than from the theme, i.e. replacing a core-provided JS file with a custom one.

Sorry if it seems like overkill. Setting back to needs review for more consideration.

cross-posting:

i think of what remains TODO about the core/misc folder is about putting which every js/css/img (by their used module) into the core module that uses them. Off hand, i think most would go into System Module.. but that's just off hand. of course, backtracking needs to be done.

I would hope this would help with the individual core module maintainers -- rather than just keeping a blank pile of asset/js | asset/img | asset/css. Making it clearer for those working with feature branches, etc. of what connects to where..

So there are two questions here:

1. Whether the file location is public API or internal API. We should discuss that in #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation.
2. Whether the impact of the change outweighs the disruption, even if we do decide asset locations are only internal API. If it turns out this is internal API, then we should discuss. At the moment it looks like the subsystem maintainers consider this worth the disruption, but the committers are hesitant. So, let's explicitly document the impacts and disruptions in the summary once we decide #1.

Postponing on #75 to make sure we don't fragment the discussion. Thanks everyone.

Change directory structure for JavaScript files this was what i was cross posting from.

#72 is still my position on this - we should look at how feasible it is to do a backwards compatibility layer for the old paths given it's a fixed number.

We should also document that file locations for assets are @internal - i.e. we let you mess with things via override stuff in .info.yml and alter hooks, but that doesn't mean we promise that the data structures you're modifying don't change, especially given other modules or base themes could be altering the same things anyway.

The bc layer would allow modules to work on both 8.1.x and 8.2.x until 8.2.x is released. We could then even remove that bc layer in 8.3.x or 8.4.x given it's an internal API change since it's going to be an ugly hardcoded mapping at best.

We could check if an alter/override is using the old path and hot swap it for the correct override/alter as a BC layer but may want to wait on catch's changes to the aggregation to get in?

#1014086: Stampedes and cold cache performance issues with css/js aggregation Since they may conflict if that BC is implemented, thoughts? maybe postpone on that?

Updated .eslintignore file as suggested in #98, thanks!

Opened #3432601: Add deprecation/bc support for library-overrides when files are moved for deprecation/bc support.