This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895

Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom
Florian-R:rollup
Closed

[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom
Florian-R:rollup

Conversation

@Florian-R

@Florian-RFlorian-R commented Nov 16, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.

TBD:

  • Fix tests
  • Tests on real world app to see it fixes Brunch and Gulp issues
  • Check for deps require
  • Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js
  • If possible, get rid of coffee extensions (see inline comments)
  • Check if coverage is still OK.

cc @embs@shvaikalesh

@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R commented Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit

Florian-R
Florian-R commented Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee Outdated

@Florian-RFlorian-RNov 16, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.

@shvaikaleshshvaikaleshNov 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.

Florian-R
Florian-R commented Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.

A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.

Florian-R
Florian-R commented Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee Outdated

@Florian-RFlorian-RNov 16, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.

@shvaikaleshshvaikaleshNov 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.

@paulmillr

paulmillr commented Nov 16, 2016

Copy link
Copy Markdown
Contributor

👍

@Florian-R

Florian-R commented Nov 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Added some commits, I got rid of the coffee extensions, and cleaned remaining require.

I've updated my first post, the bundle is way smaller (didn't check gzip tough).

I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.

@shvaikalesh

shvaikalesh commented Nov 17, 2016

Copy link
Copy Markdown
Contributor

Outstanding job, @Florian-R! Code looks so much better now.

shvaikalesh
shvaikalesh reviewed Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee Outdated

@shvaikaleshshvaikaleshNov 17, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.

@Florian-RFlorian-RNov 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.

@shvaikalesh

shvaikalesh commented Nov 17, 2016

Copy link
Copy Markdown
Contributor

Also, we should remove all 'use strict' declarations, because es6 modules are always strict.

vendethiel
vendethiel reviewed Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee Outdated

@vendethielvendethielNov 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.

@Florian-RFlorian-RNov 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.

@vendethielvendethielNov 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!

@Florian-R

Florian-R commented Nov 17, 2016

Copy link
Copy Markdown
ContributorAuthor

Also, we should remove all 'use strict' declarations, because es6 modules are always strict.

Make sense, done for the src directory for now.

@Florian-R

Florian-R commented Nov 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.

Florian-R
Florian-R commented Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
grunt.registerTask 'test', 'mochaTest:native'
grunt.registerTask 'test:jquery', 'mochaTest:jquery'
grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']

@Florian-RFlorian-RNov 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.

Florian-R
Florian-R commented Nov 23, 2016
View reviewed changes
Comment threadpackage.json
"plugins": [
"transform-es2015-modules-commonjs"
]
}

@Florian-RFlorian-RNov 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc

Florian-R
Florian-R commented Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
'coffee-script/register'
'coffee-coverage/register-istanbul'
->
global._$coffeeIstanbul = {}

@Florian-RFlorian-RNov 23, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like

if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};

in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.

Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)

Florian-R
Florian-R commented Nov 23, 2016
View reviewed changes
Comment threadpackage.json
"grunt-cli": "~0.1.13",
"grunt-coffeelint": "~0.0.15",
"grunt-contrib-clean": "1.0.0",
"grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",

@Florian-RFlorian-RNov 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.

@Florian-RFlorian-R mentioned this pull request Nov 24, 2016
Open
@shvaikaleshshvaikalesh mentioned this pull request Sep 11, 2017
Open
@Florian-R

Florian-R commented Sep 11, 2017

Copy link
Copy Markdown
ContributorAuthor

Closing per #900

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

Reviewers

2 more reviewers
@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1

4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content

This repository was archived by the owner on Jun 15, 2023. It is now read-only.

[WIP] - Switch to Rollup - #895
Closed
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Closed
[WIP] - Switch to Rollup#895
Florian-R wants to merge 4 commits into
chaplinjs:masterfrom 
Florian-R:rollup
Conversation
@Florian-R
@Florian-RFlorian-R
commented
Nov 16, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
This is one should fixes#894. For now, all tests explode because I couldn't figure a way to compile the code correctly with grunt-mocha-test, but I'd like to grab early comments.
TBD:
  •  Fix tests
  •  Tests on real world app to see it fixes Brunch and Gulp issues
  •  Check for deps require
  •  Check bundle size - Generated with grunt-compare-size :
-255216 build/chaplin.js
-4992 build/chaplin.min.js

  •  If possible, get rid of coffee extensions (see inline comments)
  •  Check if coverage is still OK.
cc @embs@shvaikalesh
@Florian-R
Update Coffeescript
e36f5fa
@Florian-R
Remove coffee-script/register in mochaTest:native
372667f
This was not needed, the coverage already provide this hooks.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin/composer.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just stumble upon the external option of rollup, I'll change this one in a future commit
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super fluent with ES6 modules, almost always used CommonJS. Cannot find a way have the import inlined in the export as were the require. Let me know if this could be done in a nicer way.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ES6 modules can do export { foo } from 'bar.coffee', but that won't help here. Furthermore, we should be careful not break CommonJS users because of default thing.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadGruntfile.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests breaks, because we babel-register had a extension hook but it expect .js file.
A simpler approach could be compile our files before running test in a tmp dir, but it may breaks coverage and will be slower and dirtier.
Florian-R
Florian-R
commented
Nov 16, 2016
View reviewed changes
Comment threadsrc/chaplin.coffee
Outdated

@Florian-RFlorian-RNov 16, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to keep extensions, else rollup doesn't resolve the files. See rollup/rollup#1052 (comment). Let me know if your cool with this, else I'll try to use rollup-plugin-node-resolve.
@shvaikaleshshvaikaleshNov 16, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give rollup-plugin-node-resolve a try, .coffee does not look very nice to me.
@paulmillr
paulmillr
commented
Nov 16, 2016

Copy link


Copy Markdown

Contributor
👍
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Added some commits, I got rid of the coffee extensions, and cleaned remaining require.
I've updated my first post, the bundle is way smaller (didn't check gzip tough).
I'd work on getting the tests to pass, in the meantime, @embs could you do a quick check with my rollup-build branch and confirm it fixes your gulp setup? I can confirm this one works fine with Brunch.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Outstanding job, @Florian-R! Code looks so much better now.
shvaikalesh
shvaikalesh
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/application.coffee
Outdated

@shvaikaleshshvaikaleshNov 17, 2016•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that we should remove .coffee here.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong search and replace, I'll revert.
@shvaikalesh
shvaikalesh
commented
Nov 17, 2016

Copy link


Copy Markdown

Contributor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
vendethiel
vendethiel
reviewed
Nov 17, 2016
View reviewed changes
Comment threadsrc/chaplin/lib/utils.coffee
Outdated

@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the require was intended. There's a dependency cycle between utils and mediator.
@Florian-RFlorian-RNov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check but I'm pretty sure Rollup handle circular dependencies.
@vendethielvendethielNov 17, 2016

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, amazing then :-)!
@Florian-R
Florian-R
commented
Nov 17, 2016

Copy link


Copy Markdown

ContributorAuthor
Also, we should remove all 'use strict' declarations, because es6 modules are always strict.
Make sense, done for the src directory for now.
@Florian-R
Florian-R
commented
Nov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Added one commits for the test. I had to use a tmp dir to compile the coffee. See benbria/coffee-coverage/issues/82 for the rationale. Let me know if your good with this approach, and I'll tweak the code to avoid a double compilation.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 grunt.registerTask 'test', 'mochaTest:native'
 grunt.registerTask 'test:jquery', 'mochaTest:jquery'
 grunt.registerTask 'test', ['clean', 'instrument', 'coffee:test', 'mochaTest:native']
 grunt.registerTask 'test:jquery', ['clean', 'instrument', 'coffee:test', 'mochaTest:jquery']
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part need some rework.
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "plugins": [
 "transform-es2015-modules-commonjs"
 ]
 }
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined this to avoid a .babelrc
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadGruntfile.coffee
 'coffee-script/register'
 'coffee-coverage/register-istanbul'
 ->
 global._$coffeeIstanbul = {}
@Florian-RFlorian-RNov 23, 2016•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly, but needed. istanbul add something like
if(typeof_$coffeeIstanbul==='undefined')_$coffeeIstanbul={};
in every modules. This breaks badly with babel-register which wrap the code in a function with a use strict pragma.
Edit: In all fairness, this comes indeed from coffee-coverage. I've proposed a fix here benbria/coffee-coverage#82 (comment)
Florian-R
Florian-R
commented
Nov 23, 2016
View reviewed changes
Comment threadpackage.json
 "grunt-cli": "~0.1.13",
 "grunt-coffeelint": "~0.0.15",
 "grunt-contrib-clean": "1.0.0",
 "grunt-contrib-coffee": "florian-r/grunt-contrib-coffee",
@Florian-RFlorian-RNov 23, 2016

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WIP pending a new release of grunt-contrib-coffee. The current version use an old version of coffee-script.
@Florian-RFlorian-R
mentioned this pull request
Nov 24, 2016

Open

@shvaikaleshshvaikalesh
mentioned this pull request
Sep 11, 2017
 Open

@Florian-R
Florian-R
commented
Sep 11, 2017

Copy link


Copy Markdown

ContributorAuthor
Closing per #900
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


2 more reviewers

@vendethielvendethielvendethiel left review comments
@shvaikaleshshvaikaleshshvaikalesh left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Incompatibility upgrading to 1.2.0 from 1.1.1


4 participants

@Florian-R@paulmillr@shvaikalesh@vendethiel