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

[RFC] fix #270, support appendTsSuffixTo config #354

Merged
merged 4 commits into from
Nov 4, 2016
Merged

[RFC] fix #270, support appendTsSuffixTo config #354

merged 4 commits into from
Nov 4, 2016

Conversation

HerringtonDarkholme
Copy link
Contributor

@HerringtonDarkholme HerringtonDarkholme commented Nov 2, 2016

Tests need more handling. Vue loader will produces absolute file path that is path dependent.
And it even produce hash that is machine specific! How can I handle these cases?

Added _flaky mark to vue's test

@HerringtonDarkholme HerringtonDarkholme changed the title fix #270, support appendTsSuffixTo config [WIP] fix #270, support appendTsSuffixTo config Nov 2, 2016
@johnnyreilly
Copy link
Member

Vue loader will produces absolute file path that is path dependent.
And it even produce hash that is machine specific!

Don't worry; our test pack already caters for this

/**
 * replace the hash if found in the output since it can change depending
 * on environments and we're not super interested in it and normalize
 * line endings
 **/
function cleanHashFromOutput(stats, webpackOutput) {
    if (stats) {
        glob.sync('**/*', { cwd: webpackOutput, nodir: true }).forEach(function (file) {
            var content = fs.readFileSync(path.join(webpackOutput, file), 'utf-8')
                .split(stats.hash).join('[hash]')
                .split('\r\n').join('\n');
            fs.writeFileSync(path.join(webpackOutput, file), content);
        });
    }
}

@johnnyreilly
Copy link
Member

Is this ready for me to take a look at? I've been holding off until you give me the go-ahead.

@HerringtonDarkholme
Copy link
Contributor Author

HerringtonDarkholme commented Nov 4, 2016

Yes, I have had flaky mark to vue's test. I think as long as the test does not exit with error. It is ok.

The problem is cleanHashFromOutput is solely for output.txt. Vue loader produces filepath in compiled bundle.js.

@HerringtonDarkholme HerringtonDarkholme changed the title [WIP] fix #270, support appendTsSuffixTo config [RFC] fix #270, support appendTsSuffixTo config Nov 4, 2016
if (!hotAPI.compatible) return
module.hot.accept()
if (!module.hot.data) {
hotAPI.createRecord("data-v-39ef11c7", __vue_options__)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, the hash code is even emitted in bundle.js

@johnnyreilly
Copy link
Member

Ah - in that case we can probably amend the test pack to cater for hashes in the bundle output as well. Actually this is kind of handy as this is the first example of a hot module reloading test in the test pack that I'm aware of. But I can take a look at that later

@johnnyreilly
Copy link
Member

Looks good to me 👍

@johnnyreilly johnnyreilly merged commit 5359d63 into TypeStrong:master Nov 4, 2016
@HerringtonDarkholme
Copy link
Contributor Author

Thanks!

@johnnyreilly
Copy link
Member

My pleasure; I've invited you to join TypeStrong so you'd have commit rights on ts-loader.

In order that we can release I think we just need to do the following:

  • update the docs to include details about the new option. Probably worth providing a quick example of how to use this with vuejs for other people to find
  • follow the standard instructions here

@johnnyreilly
Copy link
Member

Do you fancy handling that? I'd propose that this should become ts-loader v1.1.0

@HerringtonDarkholme
Copy link
Contributor Author

I think I would provide the doc part. But releasing version goes to you 😉

@johnnyreilly
Copy link
Member

That's fine 😄 - let me know when the docs are updated and I'll handle it

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.

2 participants