-
Notifications
You must be signed in to change notification settings - Fork 18
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
Use gulpfile to building things & introduce the text-encoding for testing under nodejs. #17
base: master
Are you sure you want to change the base?
Conversation
Unfortunately, I don't think Gulp is an acceptable build system here. All of the other libraries I've been playing with are using Grunt, not gulp, so I would not feel comfortable with a gulp build system. |
@@ -1,7 +1,5 @@ | |||
define(function(require) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not changing this to node-only style module syntax. If the syntax is to change, it's more likely to be UMD module style or possibly ES6 modules. (I've mostly been holding off in the hopes that ES6 modules will come to save us all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was to babel
& browserify
to do the code prepossessing, and use require is the easiest way to do that, if you think use ES6 module is necessary, we could use it later.
Also, for future reference, your commits are way too coarse-grained. One commit per issue. |
For getting the pull request works, there must be such a big change, except the stringview.js, |
@jcranmer Anyway, please at least clone my fork & running the test & have a look at the effect. |
As I mentioned earlier: I really would rather be using Grunt instead of Gulp. And the main reason for this is that the other libraries that we are using or wish to use in comm-central (e.g., ical.js) are using Grunt, and so it makes sense to standardize on that (as well as Mocha tdd based testing), so that it would ease future integration into a potential new more JS-y comm-central build system. |
@jcranmer Please also consider the gulpfile, |
I've update the pull request, maybe easier to review. |
You've still not addressed all of the concerns. Saying that gulp "is just a building system" means you're missing the point of what I'm saying. You're basically trying to change the configuration of a system to suit your needs without considering the needs of other, extant uses of the library. I can already tell that even the new patch will still break the import into Thunderbird. JSMime should already work with require.js and AMD modules (indeed, that's what the test you were trying to test was in fact doing). This makes me highly wary of your changes to support a library whose main purposes is to make Node modules work in the browser. As for development, I've done all of my testing using a localhost server with the moch/chai tests in use by Firefox. E.g., the URL http://localhost/source/jsmime/test/browser/amd lets me run all of the tests (and find lots of bugs in the test driver, since my IDNA tests in another WIP patch uses lots of weird Unicode strings). And the only reason it requires a test server is because the CORS rules on file:// suck :-( In short, you're basically completely changing the API of the project without trying to ensure that the current users of the API are still compatible. |
FWIW (since I know you can build Thunderbird), the way I test that code works in Thunderbird is: DEST=/src/trunk/comm-central/mailnews/mime/jsmime
node build/build.js
# Clean out the old import of jsmime
rm -rf $DEST/*
# Copy files to the destination
cp LICENSE README.md $DEST
cp dist/jsmime.js $DEST
find test -type f ! -path 'test/browser/*' | xargs tar c | tar x -C $DEST If running xpcshell-tests causes a test fail, then the change cannot be accepted as it would break Thunderbird. This requirement is non-negotiable. |
@jcranmer So if that not breaks thunderbird? The patch can be accepted? From my design, it's woun't break the thunderbird, cause I am still output a UMD version of jsmime.js |
The header is something like this: |
@jcranmer OK, I figure out a way to keep the old tests still running. waiting for tune. |
@jcranmer I've updated the pull request, now both nodejs test cases & firefox testcases are running. |
7124824
to
2d29a89
Compare
…same copy of test cases. To do this, we need to using the same text-encoding for both nodejs & browser. And in the future, we could easily running jsmime in worker, and add UTF7 support. Support for code coverage inspect.
The firefox browser passed all tests, including AMD/Globals. The following result is for nodejs, only GB18030 decoding is incorrect, there is two case failure, Along the nodejs testcase, the code coverage result are coming out.
|
@jcranmer any time? |
Now we could be able to introduce UTF7 and other needed encodings when necessary,
Cause all the encodings are implement in pure js, so we could be able to running jsmime in the worker.