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

proposed changes #37

Closed
joeypedicini92 opened this issue Mar 26, 2018 · 10 comments
Closed

proposed changes #37

joeypedicini92 opened this issue Mar 26, 2018 · 10 comments

Comments

@joeypedicini92
Copy link

Hey guys, first off great work on this, has been a huge help. However, I tried pulling down version 2.0 and the current master and I was having issues with EMFJS and WMFJS being undefined modules.

Because I didn't want to deal with that at the time I just pulled v1.0 (pre-typescript) and that was a bit more promising, but I did need to make the following changes to get it working:

screen shot 2018-03-26 at 12 49 09 pm

Just letting you guys know if you want to merge the changes in. Making logging more easily configurable would be a great change. And just adding module.exports to the rtf.js made it "requireable" as a module. And that parser.version check doesn't really make sense to me. Obviously I'm not as familiar with the code, but I'm not quite sure what the point of that is. It would throw an error on every RTF file I tried, and simply removing it seemed to parse the RTFs just fine.

Anyway feel free to reach out to discuss.

Best

@zoehneto
Copy link
Collaborator

Thanks for the comments @joeypedicini92 :)

I think there are multiple things to discuss here:

  1. rtf.js version 1: this is only meant as a save point for pre existing users while I was making big changes in terms of typescript, modules bundling, build steps etc. We don't work on this version any more and instead focus on what is currently in master.
  2. requiring modules: version 1 is exclusively meant to be loaded in the browser using a script tag which is why it can't be required. The new version publishes a UMD bundle which works when loading it in the browser as a script and should support loading as a module with some caveats:
    • requiring it from node will not work, since it depends on browser only libraries (jquery, jquery.svg.js), it can be used with JSDOM though (see the file test/utils.js for an example).
    • the rtf.js bundle requires modules named 'WMFJS' and 'EMFJS'. Now if we published those to npm with the correct 'main' property in package.json it would work but as it stands now your module loader probably can't find the modules because they are all in the same directory and the bundle name doesn't match the module name. If you can share what you are working on or provide a minimal example of your specific setup I'd be happy to help you get it working (might be something fixable with a webpack config entry, renaming the bundles etc.). Right now I have only used the UMD bundle to get it working as a script the way it did before, I have not used it as a proper module yet.
  3. logging configuration: you can simply do RTFJS.loggingEnabled = false for 1.0 or RTFJS.loggingEnabled(false) for the new version. It works the same way with EMFJS and WMFJS and should also work if you require it (var rtf = require('RTFJS.bundle.js'); rtf.loggingEnabled = false;)
  4. parser.version: the rtf spec requires every rtf file to declare that it is version 1 so this should not be an issue. Can you share an example file?

@joeypedicini92
Copy link
Author

Thanks for the reply @zoehneto

I completely understand v1 isn't supported anymore, I was mainly voicing a suggestion that this library could be used as a node module (it was easier to do that against v1 which is why I went that route) and the changes I made to use it in that way.

We are running this on a browser, but as part of an Ember SPA, so it makes more sense to follow the pattern of importing modules in our js files than loading scripts on our index page. I think that's a more common pattern and something that should be supported by the library.

As far as providing a sample RTF I can't do that since they contain personal data. But the RTF source is version 1...but that code is expecting the parser.version to be null, so when it comes in as 1 it throws an error...can you explain how that works?

@zoehneto
Copy link
Collaborator

I'll have to think about getting the import to work without ugly hacks.

As far as the version check is concerned I was thinking of the wrong piece of code. The part you removed checks that no rtf version was previously specified. The reason is, that the RtfDestination has to be the first destination in the file (\rtf) and it may only be specified once. The rtf destination has the version parameter which sets the parser version. So if the RtfDestination is called and a parser version is all ready specified, you probably have multiple rtf destinations which according to the spec is invalid.

@zoehneto
Copy link
Collaborator

I just pushed the branch 'module-import' which should allow you to load rtf.js with require (at the moment this means you will also get wmf.js and emf.js, optionality is currently not implemented). Could you please test whether that solves your issue?

@zoehneto
Copy link
Collaborator

I have now also added the jQuery require so rendering documents without images should work if you require rtf.js . I still have to add support for jquery-svg.

@joeypedicini92
Copy link
Author

Great thanks for the quick turnaround! I should have sometime today to test I'll let you know how it goes

@zoehneto
Copy link
Collaborator

Great, I have now also fixed the jquery-svg issue. I now provide that with a UMD header in the dist folder. As long as you keep everything in the dist folder together wherever you are copying it, requiring should work. I have tested it in an angular project (which uses webpack through the angular cli) and it works for me with the following code (using ES 6 imports, but require should work aswell):

import * as WMFJS from '../../rtf.js/WMFJS.bundle';
import * as EMFJS from '../../rtf.js/EMFJS.bundle';
import * as RTFJS from '../../rtf.js/RTFJS.bundle';

....

    stringToBinaryArray(string) {
        var buffer = new ArrayBuffer(string.length);
        var bufferView = new Uint8Array(buffer);
        for (var i=0; i<string.length; i++) {
            bufferView[i] = string.charCodeAt(i);
        }
        return buffer;
    }
    
    rtfRender(){
        RTFJS.loggingEnabled(false);
        WMFJS.loggingEnabled(false);
        EMFJS.loggingEnabled(false);

        try {
            var doc = new RTFJS.Document(this.stringToBinaryArray(this.rtf));

            var meta = doc.metadata();

            doc.render().then(function(htmlElements) {
                console.log(meta, htmlElements);
            }).catch(error => console.error(error))
        } catch (error){
            console.error(error)
        }

....

@joeypedicini92
Copy link
Author

joeypedicini92 commented Mar 27, 2018

I had to create an index.js file, and remove the main: 'rtf.js' line from package.json to be able to do something like:

import { RTFJS } from 'rtf.js'

index.js:

const WMFJS = require('./dist/WMFJS.bundle');
const EMFJS = require('./dist/EMFJS.bundle');
const RTFJS = require('./dist/RTFJS.bundle');

module.exports = {WMFJS, EMFJS, RTFJS};

there's probably a better way using typescript...

@zoehneto
Copy link
Collaborator

I have added an index.js and corrected the main attribute in package.json so everything should work correctly now.

It would be great if you could test it in your setup, then I can merge module-import into master.

@zoehneto
Copy link
Collaborator

I have just merged the branch and published the current state to npm, which should make working with the library a lot easier.

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

No branches or pull requests

2 participants