OfficeDev / office-js

A repo and NPM package for Office.js, corresponding to a copy of what gets published to the official "evergreen" Office.js CDN, at https://appsforoffice.microsoft.com/lib/1/hosted/office.js.
https://learn.microsoft.com/javascript/api/overview
Other
688 stars 95 forks source link

Remove CDN Dependency and make NPM module self sufficient #62

Closed craftytrickster closed 6 years ago

craftytrickster commented 6 years ago

Many of the office-js tutorials online recommend using the script from the CDN as seen here:

<script src="https://appsforoffice.microsoft.com/lib/1/hosted/office.js"></script>

Using a CDN is not an option for us, because we need everything to be self contained with no external network dependencies. Unfortunately, when we try to require this NPM package inside our project, the network traffic shows that the package still makes the same CDN calls to your hosted files (defeating the whole point of even having a node module in the first place).

Please help assist us in making this module self sufficient without any need for CDN calls at runtime.

Thank you.

Zlatkovsky commented 6 years ago

Could you show a Fiddler trace? What is the file that you see it reaching to the CDN for?

yuliasavinkova commented 6 years ago

On my set up I have many failed requests to the following locations in the network tab: https://static2.sharepointonline.com/ https://telemetryservice.firstpartyapps.oaspapps.com

craftytrickster commented 6 years ago

Even running the most basic setup possible and trying to include the office.js entry file fails, with the same outgoing requests as @yuliasavinkova

For example, if you replace the script tag in the below microsoft tutorial with your node_modules/@microsoft/office-js/dist/office.js file, it still fails.

https://docs.microsoft.com/en-us/office/dev/add-ins/excel/excel-add-ins-get-started-react

yuliasavinkova commented 6 years ago

It also requires us to manually copy this files to the build (since office.js tries to locate them) and even after all this it errors in excel: @microsoft/office-js/dist/office.debug.js @microsoft/office-js/dist/en-us/office_strings.js @microsoft/office-js/dist/ariatelemetry/aria-web-telemetry.js @microsoft/office-js/dist/excel-win32-16.01.debug.js

Zlatkovsky commented 6 years ago

I think there are two things under discussion here:

  1. Are there outgoing request? Answer seems to be "yes".

  2. Should you be able to copy the office.js file directly to somewhere, in the absence of the other surrounding files? The answer to that is NO, you have to copy the whole directory (so that excel-win32-16.01.debug.js and the other many dependencies are included). The office.js file itself is just a loader, so it needs to be surrounded by the other files. This one is (for now, anyway) a "won't fix". @craftytrickster , could you try to copy the entirety of the node_modules/@microsoft/office-js/dist/ directory?

Now, back to question 1: @yuliasavinkova or @craftytrickster, when you say "I have many failed requests to the following locations in the network tab", is there any harm from those failed requests, or is it more of a fire-and-forget? I.e., it there anything that doesn't work as a result of these failed requests, or do you think it's just overzealous reporting? (still not ideal, but just trying to access the situation).

Thanks!

Zlatkovsky commented 6 years ago

(Also, on a related note: someone just recently asked a question about what telemetry is captured, and where does it go to; the related thread (in progress work-item-wise) is https://github.com/OfficeDev/office-js/issues/61).

craftytrickster commented 6 years ago

Thank you for your response. The directory is being included, in fact, that is the whole issue - instead of acting like a loader, it's going out to the CDN.

Zlatkovsky commented 6 years ago

Hm. Let me make sure we're on the same page.

In my expectation, if you were to copy the entirety of https://www.npmjs.com/package/@microsoft/office-js to a local folder -- and hence get something like https://unpkg.com/@microsoft/office-js@1.1.4/dist/ in the "dist" folder of said local folder -- that should be able to work offline. It sounds like there might be some failed requests happening, and we can explore that. But as a baseline: are you able to make a local office-js inside of a directory work?

craftytrickster commented 6 years ago

The directory was already locally on the disk (inside node_modules) as is the expected result for any package fetched via npm install. I am able to easily include React and other dependencies in my project without any problem, and I am very experienced in TypeScript/JavaScript, so this issue is not due to my ignorance on how to include a package. (I know you did not insinuate that, but since so many errors are typically the result of user error, I wanted to remove any doubts).

We then followed the instructions as shown in the README as <script src="node_modules\@microsoft\office-js\dist\office.js"></script> in our index.html, and it still fails to initialize. (Instead of trying to find files it depends on locally, it still tries to use the CDN, and not just for telemetry). I do not have access to a JS aware version of Office on my computer (I only have 2010) so I will see if my colleague experiencing the issue can provide more information, but there might be a delay.

I can confirm though that if you try to run the most basic Hello World example possible using this dependency, and you look at the outgoing network connections, you will clearly see what I mean.

It might be a good idea for your team in general to have an /examples folder in your repo, with a simple hello world project setup with a correct package.json (and webpack/parceljs if applicable) that you could point to as a reference for how things should be exactly setup, since the instructions to do <src script="node_modules\@microsoft\office-js\dist\office.js"></script> seem to not be enough.

Thank you for your prompt responses.

Zlatkovsky commented 6 years ago

@craftytrickster, thanks for the details. Let me investigate, and also see if we can get a sample / documentation created for this. Adding @kbrandl to the assignees; Kim, let's work on this together.

craftytrickster commented 6 years ago

Any update on this?

nitslucky commented 6 years ago

When i try to remove this: <script src="https://appsforoffice.microsoft.com/lib/1/hosted/office.js"></script> from the index.html file and add it locally from the node modules folder : <script src="../node_modules/@microsoft/office-js/dist/office.js" type="text/javascript"></script> its throwing error in console: error

mlafleur commented 6 years ago

That isn't an issue with office.js itself but how you're serving the library. The error is telling you that https://localhost:3002/node_modules/@microsoft/office-js/dist/office.js is either failing to return anything (thus the 404).

You need to make sure the javascript it accessible to your page. How you go about configuring that will depend on your app/server. Typically there will be some mechanism for serving "static files". For example, with Node/Express it would be app.use(express.static(path));.

craftytrickster commented 6 years ago

He shouldn't need to host and serve the library on a server at all. Since this is an npm library, presumably to be used in front end html+css scenario, it should just be able to be bundled with the final bundled/minified js that he is likely serving.

craftytrickster commented 6 years ago

If not, there is no reason for this npm library to even exist, because ultimately all it is doing now is pointing to an external cdn to be accessed at runtime.

nitslucky commented 6 years ago

I am referencing to the js file as it is written in the document and its the same referencing like any other file from the node modules. And others are being loaded in the app, So it should work too

Zlatkovsky commented 6 years ago

Let me try to create a sample project over the next couple of days, and I'll get back to you.

@craftytrickster , to be clear, this is not a UMD or CommonJS module, in the way that most NPM packages are. The latter is on our backlog, but this package isn't it (at the moment, anyway). Rather, this is simply a package that hosts a copy of the same files as there are on the CDN. The intent is to allow users to have their own non-CDN copy, that they would then statically serve from their own website rather than the CDN. It is primarily meant for the in-house add-in (esp. the behind-a-corporate-firewall scenario), as the Store policy currently requires that add-ins reference the CDN for production add-ins.

But as I said, let me get back to you in a couple days -- and I'll also work with our documentation team to prioritize getting this documented, to explain what the NPM package is or isn't today.

Best! ~ Michael

craftytrickster commented 6 years ago

Thanks for the explanation.

cskr11 commented 6 years ago

Let me try to create a sample project over the next couple of days, and I'll get back to you.

@craftytrickster , to be clear, this is not a UMD or CommonJS module, in the way that most NPM packages are. The latter is on our backlog, but this package isn't it (at the moment, anyway). Rather, this is simply a package that hosts a copy of the same files as there are on the CDN. The intent is to allow users to have their own non-CDN copy, that they would then statically serve from their own website rather than the CDN. It is primarily meant for the in-house add-in (esp. the behind-a-corporate-firewall scenario), as the Store policy currently requires that add-ins reference the CDN for production add-ins.

But as I said, let me get back to you in a couple days -- and I'll also work with our documentation team to prioritize getting this documented, to explain what the NPM package is or isn't today.

Best! ~ Michael

Hi, Please let me know if there are any updates for this. I am also facing the same after following the instructions in README

Zlatkovsky commented 6 years ago

@cskr11 , I'm re-following up re. more official documentation for this. But in terms of how to make it work: the package essentially is just a copy of what's on the CDN. You can copy over the dist folder as-is and put it wherever you want (e.g., in your assets directory), and then reference it as https://mysite.com/assets/office.js in your html page. It's really just a convenient way for us to distribute a copy of what's on the CDN to people who want it for offline debugging, or firewalled scenarios, etc.

cskr11 commented 6 years ago

@Zlatkovsky Thanks. It is working. I hope we have a solution for this as it will be difficult in terms of keeping it update with the latest version of office.js.

Zlatkovsky commented 6 years ago

FYI, the readme/explanation is nearly in. It is fixed by pull request https://github.com/OfficeDev/office-js/pull/260.

@cskr11 , if you are using office.js from the CDN, you don't need this package at all -- you can instead get the types from @types/office-js. Conversely, if you are using office.js from this package, then it will always be in a self-consistent state with its own dist/office.d.ts. So the "keeping it updated with the latest version of office.js" should be a non-issue in either case.

See the text that's being updated by the pull request and the comments therein, and hopefully that should clarify things.

kbrandl commented 6 years ago

We've updated the README to provide more guidance about the Office.js NPM package (target scenarios, how to use it, etc.). Closing this issue, per @Zlatkovsky's request.

rvanlaak commented 6 years ago

Also see #271

Great news that the CDN is not a requirement anymore 🎉 An internal check on basePath to load translation strings still make it impossible to use bundlers like webpack to require the library though. The following error gets thrown:

Office Web Extension script library file name should be office.js or office.debug.js.

Especially given that we're writing an ES6 (instead of a TS) project. We did try to include @types/office-js with our webpack externals, but @types/office-js as being TS thereby obviously can not get imported.

It seems a base path gets determined based on the script tag (line 246), but bundlers like webpack don't work with script tags anymore. Can the filename check be removed / changed?