Skip to content

Fix react-templates #1

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.

Sign up for GitHub

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

Open
awithersdd wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix react-templates #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5b277b
fix this to run when it needs to
Sep 11, 2016
75528ea
post build
Sep 11, 2016
d180792
Final changes, this always emits require as changes were made to bh-f…
Oct 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions dist/reactTemplates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,16 @@ function generateProps(node, context) {
props[propKey] = convertText(node, context, val.trim());
}

// Note: The correct way to handle require being output in generated
// templates is to ensure the output is processed by webpack.
if (node.name === 'img' && propKey === 'src') {
let evaluated = props[propKey];
if (/^"\/?bhf-assets/.test(evaluated)) {
evaluated = 'require(' + evaluated.replace(/^"\/?bhf-assets/, '"bhf-assets') + ')';
let nv = val.trim();
// Remove any leading / so module aliases are easier, the
// code should probably be updated but this is currently easier.
if (nv[0] === '/') {
nv = nv.slice(1);
}
props[propKey] = evaluated;
props[propKey] = `require('${ nv }')`;
}
Copy link

@erichulburd erichulburd Oct 12, 2016

Choose a reason for hiding this comment

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

The existing implementation will only work on src with /bhf-assets/... or bhf-assets/....

My concern here is, what if we have a an img src with https://www.example.com/photo.jpg?

Do we have img src's hosted at other 1st level paths? Development-wise, we are pointing references to img's and background url-s with bhf-assets.

Copy link
Author

@awithersdd awithersdd Oct 13, 2016

Choose a reason for hiding this comment

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

I reviewed every existing asset with this change, the new build also preserves webpack build warnings and errors (which will show cases like this). There are currently zero such references and that seems likely to continue.

Having spent significantly more time with webpack now the original move from assets to bhf-assets was entirely unnecessary (we could have prefixed all references automatically). I went with removing explicit bhf-assets as it didn't seem necessary.

Here is an example from build_stats.json when I put an reference in intro_apps_and_trackers.rt.html:

"errors": [
    "(undefined) ./shared/components/wizard/forms/intro_apps_and_trackers.rt.html\nModule not found: Error: Cannot resolve module 'http://www.anduin.com/anduin.png' in /usr/src/app/shared/components/wizard/forms\nresolve module http://www.anduin.com/anduin.png in /usr/src/app/shared/components/wizard/forms\n  looking for modules in /usr/src/app/node_modules\n    /usr/src/app/node_modules/http: doesn't exist (module as directory)\n[/usr/src/app/node_modules/http:]\n @ ./shared/components/wizard/forms/intro_apps_and_trackers.rt.html 7:672-715",

This also results in an exception being generated for the element contents:

e.createElement("img",{src:n(!function(){var e=new Error('Cannot find module "http://www.anduin.com/anduin.png"');throw e.code="MODULE_NOT_FOUND",e}())})

});
_.assign(props, generateTemplateProps(node, context));
Expand Down
16 changes: 9 additions & 7 deletions src/reactTemplates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,15 +245,17 @@ function generateProps(node, context) {
props[propKey] = convertText(node, context, val.trim());
}

// Only run this in the client.
// Note: The correct way to handle require being output in generated
// templates is to ensure the output is processed by webpack.
if (node.name === 'img' && propKey === 'src') {
let evaluated = props[propKey];
if (/^"\/?bhf-assets/.test(evaluated)){
evaluated = `( typeof window !== 'undefined' ? require(${evaluated.replace(/^"\/?bhf-assets/, '"bhf-assets')}) : ${evaluated})`;
}
props[propKey] = evaluated;
let nv = val.trim();
// Remove any leading / so module aliases are easier, the
// code should probably be updated but this is currently easier.
if (nv[0] === '/') {
nv = nv.slice(1);
}
props[propKey] = `require('${nv}')`;
}

});
_.assign(props, generateTemplateProps(node, context));

Expand Down