Add cache key to assets #5003

sculpt0r · 2021-12-21T11:35:41Z

What is the purpose of this pull request?

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4761](https://github.com/ckeditor/ckeditor4/issues/4761): Add cacheKey (timestamp) to CSS assets loads via editor.

What changes did you make?

Give an overview…

The previous solution #4852 changed the default behavior for CSS URLs. Each time, the URL was provided - it was turned into the absolute path via the getUrl function which adds URL calculated based on CKEDITOR_BASEPATH.

ckeditor4/core/ckeditor_base.js

Lines 172 to 175 in 10b8a21

    
           getUrl: function( resource ) { 
        
           	// If this is not a full or absolute path. 
        
           	if ( resource.indexOf( ':/' ) == -1 && resource.indexOf( '/' ) !== 0 ) 
        
           		resource = this.basePath + resource;

However, it was not always valid for products that may serve CKE4 from a specific location. The user may still want to serve its own CSS files based on the path related to the loaded page, not the editor.

Please notice, that changes in ckeditor_base.js require additional action and lead to changes in ckeditor.js:

ckeditor4/core/ckeditor_base.js

Line 13 in 10b8a21

// Simply run `grunt ckeditor-base-replace` after changing this file.

I changed almost the same places, but only appends the cache key.

Also, added tests for getUrl from the previous solution. Since @Comandeer already spends time to cover this part of the editor with the test - we may use them.

Which issues does your PR resolve?

Closes #4761 .

github-actions · 2021-12-30T09:14:34Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

KarolDawidziuk

Hi, @sculpt0r
Everything looks solid and works properly. 👍

There are some small typos to check and also I am thinking about creating an additional test. Please check my comments below.

KarolDawidziuk · 2021-12-30T15:13:23Z

tests/core/ckeditor/manual/assetscachekey.html

+
+<script>
+	( function() {
+		if ( bender.tools.env.mobile || !CKEDITOR.timestamp ) {


Currently, this test will be only run on builded version of the editor.
Maybe should we create another test with sets CKEDITOR.timestamp, which will allow checking properly adding cache key without building the editor, WDYT?

It'd be difficult to do so as timestamp should be set before resources are being loaded – so basically before loading CKEditor. As Bender manages loading the editor, it's a really non-trivial task

Generally speaking sounds like a good idea, but due to ⬆️ #5003 (comment) I'll leave it as it is.

KarolDawidziuk · 2021-12-30T15:46:21Z

tests/core/tools.js

+		},
+
+		// (#4761)
+		'test buidStyleHtml returns relative URL for passed relative URL string': function() {


Suggested change

'test buidStyleHtml returns relative URL for passed relative URL string': function() {

'test buildStyleHtml returns

The same typo is in the next 2 tests.

KarolDawidziuk · 2021-12-30T15:49:03Z

tests/core/tools.js

+				expected = '<style>' + styleText + '</style>',
+				styledStringElem = CKEDITOR.tools.buildStyleHtml( styleText );
+
+			assert.areSame( expected, styledStringElem, 'Styled text was not exact same wrapped in style elem' );


Suggested change

assert.areSame( expected, styledStringElem, 'Styled text was not exact same wrapped in style elem' );

assert.areSame( expected, styledStringElem, 'Styled text was not exact same wrapped in style element' );

Comandeer

The solution looks good, however some stylesheets are still missing a cache key after building docs, e.g.

http://localhost:9001/ckeditor4/4.17.1/examples/image2.html – tableselection,css,
http://localhost:9001/ckeditor4/4.17.1/examples/spreadsheets.html – balloontoolbar.css, default.css, dialog.css.

Could you look into it?

Comandeer · 2022-01-04T11:13:23Z

tests/core/ckeditor/manual/assetscachekey.html

+
+<script>
+	( function() {
+		if ( bender.tools.env.mobile || !CKEDITOR.timestamp ) {


It'd be difficult to do so as timestamp should be set before resources are being loaded – so basically before loading CKEditor. As Bender manages loading the editor, it's a really non-trivial task

sculpt0r · 2022-01-05T12:19:53Z

Rebased on the newest master.

sculpt0r · 2022-01-05T12:46:43Z

In my case:

http://localhost:9001/ckeditor4/4.17.1/examples/image2.html – tableselection,css,

http://localhost:9001/ckeditor4/4.17.1/examples/spreadsheets.html – balloontoolbar.css, default.css, dialog.css.

@Comandeer are you sure you have a freshly built doc version?

Comandeer · 2022-01-05T12:48:02Z

@Comandeer are you sure you have a freshly built doc version?

Yes. These were the only resources without the cache key, everything else had it 🤔 I'll check it once more.

Comandeer · 2022-01-05T13:21:14Z

Ok, it works. It seems that something's wrong with our docs build system (see #5019) and I had to force proper CKE4 version by manually checking out the branch inside repos/ckeditor-presets/ckeditor.

Comandeer

The solution looks good, I'm only missing direct tests for CKEDITOR.appendTimestamp().

sculpt0r · 2022-01-07T12:41:56Z

Rebased on the newest master

sculpt0r · 2022-01-10T07:48:18Z

@Comandeer - ready for another review.

added tests for appendNamespace

Comandeer

LGTM! I've just slightly improved API docs.

sculpt0r changed the title ~~Add manual test.~~ Add cache key to assets Dec 21, 2021

sculpt0r marked this pull request as ready for review December 23, 2021 08:44

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Dec 30, 2021

KarolDawidziuk self-requested a review December 30, 2021 12:27

KarolDawidziuk requested changes Dec 30, 2021

View reviewed changes

jacekbogdanski requested a review from Comandeer January 3, 2022 08:30

jacekbogdanski removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 3, 2022

Comandeer requested changes Jan 4, 2022

View reviewed changes

sculpt0r self-assigned this Jan 5, 2022

sculpt0r force-pushed the t/4761-2 branch from 11d8d64 to 5f4ab29 Compare January 5, 2022 12:19

sculpt0r requested review from Comandeer and KarolDawidziuk January 5, 2022 12:47

CKEditorBot unassigned sculpt0r Jan 5, 2022

Comandeer requested changes Jan 7, 2022

View reviewed changes

sculpt0r self-assigned this Jan 7, 2022

sculpt0r force-pushed the t/4761-2 branch from 4fa2995 to 95dda05 Compare January 7, 2022 12:41

sculpt0r requested a review from Comandeer January 10, 2022 07:47

CKEditorBot unassigned sculpt0r Jan 10, 2022

Comandeer self-assigned this Jan 11, 2022

Comandeer and others added 4 commits January 12, 2022 12:42

Add manual test.

7e51af5

Renamed manual test=

f48c335

Add mobile to ignore env

e149a13

Add tests for current state of

f3f6d2e

sculpt0r and others added 12 commits January 12, 2022 12:42

Modernize test for & no timestamp

ad23aac

Add test & apply cache key to styleHtml element'

10beb40

Add test case for timestamp with array of urls

03ab4c4

Add cache key to css Url via

cc875e0

Extract key cache appending logic to separate method

d452039

Refactor cache key appending funtion to be easier readable

9917c7f

Add tests for CKEDITOR.getUrl()

efadb3f

Correct typos

b5d6eb7

Add timestamp tests

d17ee14

Code style & new case

ca89dcb

Small improvements to API docs.

4cd90b8

Add changelog entries.

492b855

Comandeer force-pushed the t/4761-2 branch from 920f22e to 492b855 Compare January 12, 2022 13:40

Comandeer approved these changes Jan 12, 2022

View reviewed changes

CKEditorBot unassigned Comandeer Jan 12, 2022

KarolDawidziuk approved these changes Jan 12, 2022

View reviewed changes

Comandeer merged commit f8caf0b into master Jan 12, 2022

Comandeer deleted the t/4761-2 branch January 12, 2022 14:50

CKEditorBot mentioned this pull request Jan 12, 2022

Cache key not applied everywhere #4761

Closed

jacekbogdanski mentioned this pull request Jan 13, 2022

Add manual test verifying cache key loaded via CDN #5034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cache key to assets #5003

Add cache key to assets #5003

sculpt0r commented Dec 21, 2021 •

edited

Loading

github-actions bot commented Dec 30, 2021

KarolDawidziuk left a comment

KarolDawidziuk Dec 30, 2021

Comandeer Jan 4, 2022

sculpt0r Jan 5, 2022 •

edited

Loading

KarolDawidziuk Dec 30, 2021

KarolDawidziuk Dec 30, 2021

Comandeer left a comment

Comandeer Jan 4, 2022

sculpt0r commented Jan 5, 2022

sculpt0r commented Jan 5, 2022

Comandeer commented Jan 5, 2022 •

edited

Loading

Comandeer commented Jan 5, 2022

Comandeer left a comment

sculpt0r commented Jan 7, 2022

sculpt0r commented Jan 10, 2022

Comandeer left a comment

	getUrl: function( resource ) {
	// If this is not a full or absolute path.
	if ( resource.indexOf( ':/' ) == -1 && resource.indexOf( '/' ) !== 0 )
	resource = this.basePath + resource;

	'test buidStyleHtml returns relative URL for passed relative URL string': function() {
	'test buildStyleHtml returns

	assert.areSame( expected, styledStringElem, 'Styled text was not exact same wrapped in style elem' );
	assert.areSame( expected, styledStringElem, 'Styled text was not exact same wrapped in style element' );

Add cache key to assets #5003

Add cache key to assets #5003

Conversation

sculpt0r commented Dec 21, 2021 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

Did you follow the CKEditor 4 code style guide?

What is the proposed changelog entry for this pull request?

What changes did you make?

Which issues does your PR resolve?

github-actions bot commented Dec 30, 2021

KarolDawidziuk left a comment

Choose a reason for hiding this comment

KarolDawidziuk Dec 30, 2021

Choose a reason for hiding this comment

Comandeer Jan 4, 2022

Choose a reason for hiding this comment

sculpt0r Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

KarolDawidziuk Dec 30, 2021

Choose a reason for hiding this comment

KarolDawidziuk Dec 30, 2021

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Comandeer Jan 4, 2022

Choose a reason for hiding this comment

sculpt0r commented Jan 5, 2022

sculpt0r commented Jan 5, 2022

Comandeer commented Jan 5, 2022 • edited Loading

Comandeer commented Jan 5, 2022

Comandeer left a comment

Choose a reason for hiding this comment

sculpt0r commented Jan 7, 2022

sculpt0r commented Jan 10, 2022

Comandeer left a comment

Choose a reason for hiding this comment

sculpt0r commented Dec 21, 2021 •

edited

Loading

sculpt0r Jan 5, 2022 •

edited

Loading

Comandeer commented Jan 5, 2022 •

edited

Loading