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

Jump to bottom

added CDN settings to reaction #4316

Merged
merged 9 commits into from Jul 11, 2018
Merged

added CDN settings to reaction #4316

merged 9 commits into from Jul 11, 2018

Conversation

Akarshit
Copy link
Contributor

@Akarshit Akarshit commented Jun 11, 2018 •
edited

Resolves #4315
Impact: minor
Type: feature

Issue

After creating the production bundle from reaction, if the user wants to serve the bundled js and css files, there is no native way to do it.

Solution

Meteor provides a function to set the serving URL of the js and css files, so I am just passing the CDN's URL from setting file to Meteor.

Testing

  1. Build the production bundle from reaction using meteor build --directory "build dir"
  2. cd "build dir"/bundle/programs/server
  3. npm install
  4. cd "build dir"/bundle
  5. PORT=4000 ROOT_URL=http://localhost.com METEOR_SETTINGS="{ \"cdnPrefix\": \"cdnURL\" }" node main.js
  6. Observe from the Network tab in your browser that reaction tries to get the js and css files from the "cdnURL"

@Akarshit Akarshit requested a review from zenweasel June 11, 2018 10:22
@Akarshit Akarshit changed the title added CDN settings to rection added CDN settings to reaction Jun 11, 2018
zenweasel
zenweasel requested changes Jun 12, 2018
View reviewed changes
Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

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

Let's put this in it's own file

server/startup/index.js Outdated
@@ -26,6 +26,11 @@ SimpleSchema.defineValidationErrorTransform((error) => {
export default function startup() {
const startTime = Date.now();

// for the user who wants to serve bundled js and css files from different URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's put this in it's own file so we aren't anything extra to this file

@Akarshit
Copy link
Contributor Author

@zenweasel updated.

zenweasel
zenweasel requested changes Jun 13, 2018
View reviewed changes
server/startup/cdn.js Outdated
*
* @returns null
*/
export default function CDN() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Function naming should follow the conventions in the style guide. Specifically this should be a verb giving you some sense of what this function does and camel casing includes acronyms. e.g. setupCdn . And it should include a JSDoc

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Akarshit What's happening with this?

zenweasel
zenweasel previously requested changes Jun 20, 2018
View reviewed changes
Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

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

The JSDoc still needs work

server/startup/cdn.js Outdated
import { Meteor } from "meteor/meteor";

/**
* Sets prefix for the user who wants to serve bundled js and css
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not in the correct JSDoc format. Please look at the documentation for the correct format.

Also, javascript functions don't return null by default they return undefined which is not the same thing

@spencern
Copy link
Contributor

@zenweasel @Akarshit need another review here and to pull in the updated .snyk file from master

@Akarshit
Copy link
Contributor Author

@spencern I have merge release-1.14.0 into this, what else should I do?

@zenweasel
Copy link
Collaborator

@Akarshit I need to take another look. Will do it on Monday

@zenweasel
Copy link
Collaborator

When I follow the testing instructions I get

in the root directory of your application.
/Users/brenthoover/meteor-builds/bundle/programs/server/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:218
      throw error;
      ^

Error: Cannot find module '/imports/plugins/included/taxes-taxjar/server'
    at makeMissingError (packages/modules-runtime.js:231:12)
    at require (packages/modules-runtime.js:241:19)
    at plugins.js (server/plugins.js:1:3133)
    at fileEvaluate (packages/modules-runtime.js:339:7)
    at require (packages/modules-runtime.js:238:16)
    at /Users/brenthoover/meteor-builds/bundle/programs/server/app/app.js:106915:1
    at /Users/brenthoover/meteor-builds/bundle/programs/server/boot.js:411:36
    at Array.forEach (<anonymous>)
    at /Users/brenthoover/meteor-builds/bundle/programs/server/boot.js:220:19
    at /Users/brenthoover/meteor-builds/bundle/programs/server/boot.js:471:5
    at Function.run (/Users/brenthoover/meteor-builds/bundle/programs/server/profile.js:510:12)
    at /Users/brenthoover/meteor-builds/bundle/programs/server/boot.js:470:11

@zenweasel
Copy link
Collaborator

Ok, figured out that problems (have to rerun reaction plugins load to clean out imports). but when I bring the app up I get this:

http://localhost:4000/cdnURL/5c9381ddcbcae43f804286621e340b1b0d34f882.js?meteor_js_resource=true 404 (Not Found)

Note that I am copying and pasting your instructions from above verbatim.

@Akarshit
Copy link
Contributor Author

Akarshit commented Jul 2, 2018

@zenweasel That's weird, because I just tested the instructions again and they work.
Except that MONGO_URL also needs to be passed in the final command.

@zenweasel
Copy link
Collaborator

Yes, here is the exactly line I am pasting;

PORT=4000 MONGO_URL=mongodb://localhost:27017/meteor ROOT_URL=http://localhost.com METEOR_SETTINGS="{ \"cdnPrefix\": \"cdnURL\" }" node main.js

And I get:

http://localhost:4000/cdnURL/5c9381ddcbcae43f804286621e340b1b0d34f882.js?meteor_js_resource=true

@zenweasel
Copy link
Collaborator

I am expecting to get:

cdnURL/5c9381ddcbcae43f804286621e340b1b0d34f882.js?meteor_js_resource=true

@Akarshit
Copy link
Contributor Author

Akarshit commented Jul 2, 2018

@zenweasel This is the intended behaviour
so if you put cdnPrefix: hello, the files are served from ROOT_URL/cdnPrefix
and if you put cdnPrefix: http://rc.com, the files are served from http://rc.com/

@zenweasel
Copy link
Collaborator

Got it. Could have been clearer in the test instructions.

@zenweasel zenweasel self-requested a review July 2, 2018 05:08
zenweasel
zenweasel approved these changes Jul 2, 2018
View reviewed changes
Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

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

Tested. Verified works

@zenweasel
Copy link
Collaborator

@Akarshit We have a conflict here now

@Akarshit
Copy link
Contributor Author

Akarshit commented Jul 9, 2018

@zenweasel
Updated.

@spencern spencern merged commit 478a9a6 into release-1.14.0 Jul 11, 2018
@spencern spencern deleted the fix-4315-cdn branch July 11, 2018 18:08
@spencern spencern mentioned this pull request Jul 11, 2018
Merged
@zenweasel zenweasel mentioned this pull request Jul 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zenweasel zenweasel zenweasel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Akarshit @spencern @zenweasel