Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Refactor library to support ES201* JavaScript #206

Merged
merged 26 commits into from
May 25, 2017

Conversation

tdeekens
Copy link
Collaborator

@tdeekens tdeekens commented Mar 18, 2017

This adds rollup as a build tool and relates to issue #202.

Background & Context

Currently DOMPurity does not have a built tool in place. This is nice and often simplifies things. However, adding webpack might give the project some benefits and this issue is about to align on the potential gain of it.

This PR might change quite a couple of things and can/should be seen as a start of a discussion on how far to go with things.

  1. Change in API with more granular exports (not "just" the object) e.g. sanitize and
    • to allow import sanitize, { addHook } from 'purify'

Included in this

  1. Adding webpack with configuration for umd build
  2. Removing jshint replacing it with xo
    • to allow ES201x syntax to be linted with a default set of sane eslint rules
  3. Refactors some code to illustrate potential in changes
    • Some files where put into separate modules
    • Other changes fix linting errors while others add exceptions
  4. Rewires jsdom to test built version of purify

Open complications

  1. Mode detection (running in browser or node)
    • exports are static and early return won't do (?) which breaks script parsing in node

I tried to split up changes into sensible commits to be "easier" to digest. I hope it worked.

closes #202

@cure53
Copy link
Owner

cure53 commented Mar 30, 2017

Heya, is this already ready for merging?

@tdeekens
Copy link
Collaborator Author

No no. I was more waiting for some input. Agreement and potentially disagreements. I can continue working on it as soon as I have time whenever the general path is agreed with kina ☺️.

@tdeekens tdeekens changed the title Refactoring/es2016 Refactor library to support ES201* JavaScript May 14, 2017
@tdeekens
Copy link
Collaborator Author

@cure53 Alright, progress. The build passes again and all tooling, apart from the webpack vs. rollup debate, is in place. Next up I will have to rebase/merge with master. Which is gonna be a bit of hand picking 🐱 .

@cure53
Copy link
Owner

cure53 commented May 17, 2017

Sorry, I honestly have no idea about these things and rely on review-input from @fhemberger and @neilj :) My feedback here is as worthless as it can be.

@tdeekens tdeekens force-pushed the refactoring/es2016 branch 2 times, most recently from 7bac0cf to e65bd2b Compare May 17, 2017 22:15
@tdeekens
Copy link
Collaborator Author

No worries. I just rebased all changes over master and fixed remaining issues. I hope the commit history makes sense to review. Would love to hear thoughts from the other contributors.

@tdeekens
Copy link
Collaborator Author

Alright, rebased again on top of master to get changes from version 0.9.0 in. I'll now replace 'webpack' with 'rollup'. Any feedback so far?

@fhemberger
Copy link
Contributor

@tdeekens Sorry, I haven't worked with webpack or rollup so far, so I can't asses this PR.

@tdeekens
Copy link
Collaborator Author

No worries. I'll have a friend very familiar with it look over it maybe to give some "trust indication"?

@tdeekens
Copy link
Collaborator Author

...so karma now also runs using rollup and some other improvements have been made.

@cure53
Copy link
Owner

cure53 commented May 19, 2017

I finally managed to read the code, shall we give it a try? :D Or not yet merge ready?

@tdeekens
Copy link
Collaborator Author

Nothing much to add from my side. Give me the weekend to look over things one last time and maybe update our jsdom testing to jsdom@10.

I'll ping you once I am certain all is good. 🎈

@tdeekens
Copy link
Collaborator Author

tdeekens commented May 20, 2017

@cure53. From my side this is ready to go in. Anything you'd like addressed before hitting the button? Maybe you can run the branch against the whole browser suite before getting it merged as we don't test cross browser on PRs?

@luhmann
Copy link

luhmann commented May 22, 2017

I took a look at this as requested by @tdeekens and ran the cross-browser test-suite against the configured browsers as well as the most recent versions of Firefox, Chrome, Safari and Edge.

The tests are fine except for IE Edge 15, where this test seems to be consistently failing: https://github.com/tdeekens/DOMPurify/blob/refactoring/es2016/test/test-suite.js#L36

Output

Expected: [
	  "<a data-·._=\"foo\" href=\"#\">abc</a>",
	  "<a href=\"#\">abc</a>"
	]
	Actual: "<a href=\"#\" data-·._=\"foo\">abc</a>"
	   at Anonymous function (node_modules/qunitjs/qunit/qunit.js:2194:4)
	   at QUnit.assert.contains (test/config/setup.js:13:3)
	   at Anonymous function (test/purify.spec.js:977:9)
	   at runTest (node_modules/qunitjs/qunit/qunit.js:843:4)
	   at Test.prototype.run (node_modules/qunitjs/qunit/qunit.js:828:4)
	   at Anonymous function (node_modules/qunitjs/qunit/qunit.js:970:6)

But if I read the comment next to the test correctly, that seems to be expected. So don't know if there is any issue here.

Besides that I only found two minor issues:

  1. The karma test-config is not executable as is because there is a typo in https://github.com/tdeekens/DOMPurify/blob/refactoring/es2016/test/karma.conf.js#L73, the browser is named bs_yosemite_safari_9, but further down in the same file it is referenced as bs_yosemite_safari_8, the latter is correct for the intended configuration.
  2. There is one remaining reference to Webpack in README.md under Development and contributing

Wasn't in the mood to create a PR for things that small .. sorry 😉

Apart from that the setup looks fine to me and the application code at least looks sane. Cannot comment on functionality per se, but the tests are passing so we should be fine.

I would have one suggestion for improvement, which would be to also run the test-suite in mobile browsers as that is a feasible use-case for the library and possible with Browserstack. But I do not know if you want to support that.

@tdeekens
Copy link
Collaborator Author

Thanks bud. I'll take care of the two minor issues. The failing test is something @cure53 should comment on.

@tdeekens
Copy link
Collaborator Author

The last two commits address the issues mentioned. Leaves us with:

  1. IE Edge 15 not working - is that the case on master too?
  2. The question if we should test mobile - probably not part of this PR

Thanks everyone!

@cure53
Copy link
Owner

cure53 commented May 23, 2017

The failing test is something @cure53 should comment on.

Good catch, fixed it :D

cure53 added a commit that referenced this pull request May 23, 2017
Fixed a typo
@tdeekens
Copy link
Collaborator Author

Did you fix the yosemite typo only - as I did here too - or also the failing test? 😅

@cure53
Copy link
Owner

cure53 commented May 23, 2017

Only fixed the typo so far.

Actual: "<a href=\"#\" data-·._=\"foo\">abc</a>"

This looks safe to me. MSIE/Edge often change attribute orders. This can be added to the expectations array without risk as far as I can see.

@tdeekens
Copy link
Collaborator Author

Do you mind adding it? I'll then resolve conflicts here and we can get this in?

cure53 added a commit that referenced this pull request May 24, 2017
Fixed test for Edge 15
@cure53
Copy link
Owner

cure53 commented May 24, 2017

Done :)

@tdeekens
Copy link
Collaborator Author

Alright @cure53 clean rebase, clean merge? Merge whenever you feel like it.

@cure53 cure53 merged commit 0602ec0 into cure53:master May 25, 2017
@cure53
Copy link
Owner

cure53 commented May 25, 2017

@tdeekens Looks like the browser tests are failing now, maybe I did sth wrong?

https://travis-ci.org/cure53/DOMPurify/jobs/235933656

@tdeekens
Copy link
Collaborator Author

Oh shot. I will look into it. I ran karma locally before and thought we ran againat BrowserStack too. Can only be some configuration.

@tdeekens tdeekens deleted the refactoring/es2016 branch May 25, 2017 08:35
@tdeekens
Copy link
Collaborator Author

I just ran the karma tests locally and they pass. I also ran FF52 and IE10 against my personal BrowserStack account and they pass. I am bit confused by why the failed after the merge. The error indicates that our setup is not transpiled correctly. I'll try to find out why.

@cure53
Copy link
Owner

cure53 commented May 25, 2017

Yeah looks like it, thx :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion]: Adding webpack to allow ES201* JavaScript
4 participants