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

added html importing from js file #1528

Closed
wants to merge 1 commit into from

Conversation

stevengill
Copy link

This is inspired by the work @pierredavidbelanger did in #926

This PR is to get the following code to work

import HTML from './test.html'

Right now, that command will throw an error.

I believe it should give you a string representation of the HTML which you could then set innerHTML on some element to display it.

In the PR I referenced above, there was some discussion that importing a html file in javascript should return a path to the file in dist. That doesn't work today either and I find that less useful than return the contents of the html file.

I'm not sure the way I implemented it is the right way to do this. When a JS file is importing an html file, right now the HTMLPackager will write the file into dist as you would expect and run all of the hashing on the name + dependencies. But then when JSPackager is getting ready to write out the javascript file, it previously didn't write out the generated (String representation) of the html into the javascript file. That is what this PR fixes.

Let me know what you think.

@stevengill
Copy link
Author

@KeineLimonade how is this not backwards compatible? As far as I can see, importing an html file in JS is broken currently. (It doesn't return the name today)

What are some of the benefits of getting just the path back? The only one I see is that you can create a link to the new html page from JS. But in the component driven world of react, you don't have separate html pages anymore. Just swapping components. This PR is also very useful for angular templates and other frameworks that use a component or single page app type model.

@KeineLimonade
Copy link

Nevermind, I forgot that I have a plugin that overrides the default behavior the same way your PR will does.

@stevengill
Copy link
Author

@KeineLimonade what plugin do you use? Can you elaborate on your usecases?

@KeineLimonade
Copy link

I generate HTML files for my blog based on files with a .post extension. These files are compiled to HTML. But if they are imported from js they return their location as a string.

I simply use a local plugin that extends HTML Asset

@icopp
Copy link

icopp commented Jul 11, 2018

Personally, I would prefer an import HTML from './test.html' to output test.html to the bundle and set HTML to the path of it, the same way as with images. The fs shim already exists for getting file contents, and not having an easy way to bundle the file makes life harder for some AngularJS (1.x) projects dependent on actual separate files, not just string contents.

@devongovett
Copy link
Member

Yeah the problem with using fs.readFileSync to inline the HTML is that this does not cause any processing to occur on that asset - it just gets inlined directly. This means that the dependencies of the HTML file won't get processed.

// needs to be included in the generated javascript file
await this.writeModule(
mod.id,
`module.exports=${JSON.stringify(mod.generated.html)}`
Copy link
Member

Choose a reason for hiding this comment

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

The problem with inlining like this is that we do additional processing on the HTML output in the HTML packager, which wouldn't be accounted for here. For example, we insert additional script/style tags when you import css from js etc. This problem is related to #1456, which is about doing the exact opposite of this and inlining JS/CSS into HTML. We should be able to solve both issues with the same architecture.

@icopp
Copy link

icopp commented Jul 15, 2018

@devongovett It seems like more broadly, the problem is that there's no toggle option to switch between "put the file in the bundle and give me its URL" and "give me me the export or relevant string contents for the file" for any given import statement. Webpack mostly has the same issue, but works around it somewhat by having import X from 'some-file!loader-options-here' style syntax available.

@devongovett
Copy link
Member

Yeah IMO there needs to be a syntax similar to require.resolve for import. Then you could do something like import.resolve('./other.html') and have it resolve to a URL.

@devongovett devongovett mentioned this pull request Jul 15, 2018
@devongovett
Copy link
Member

Ok I made a new PR based on this one which supports async importing of HTML into JS, along with fake "sync" importing similar to how we handle it for WASM. Until we can actually run the HTML packager inside the JS packager to get the full HTML output inlined, this is a reasonable solution. #1732

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

Successfully merging this pull request may close these issues.

None yet

4 participants