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

refactor(loader): v1.0.0 #157

Merged
merged 8 commits into from
Dec 15, 2017
Merged

refactor(loader): v1.0.0 #157

merged 8 commits into from
Dec 15, 2017

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Dec 10, 2017

Noteable Changes

Features

url (options.url)

file.html

<body>
 <img src="path/to/file.png">
</body>

module.js

import HTML__URL__0 from 'path/to/file.png';

export default `<body>
  <img src="${HTML__URL__0}">
</body>
`

import (options.import)

file.html

<body>
  <import src="path/to/import.html">
</body>

module.js

import HTML__IMPORT__0 from 'path/to/import.html';

export default `<body>
  ${HTML__IMPORT__0}
</body>
`

template (options.template)

file.html

<body>
  <h1>${ _.hello }</h1>
</body>

entry.js

import template from './file.html';

const html = template({ hello: 'Hello World' });

document.body.innerHTML = `${html}`;

minimize (options.minimize)

BREAKING CHANGES

root (options.root)

  • Removed

attrs (options.attrs)

  • Removed (will be handled by the url plugin)

interpolate (options.interpolate)

  • Removed in favor of options.template

exportAsDefault && exportAsEs6Default (options.exportAsDefault|options.exportAsEs6Default)

  • Removed in favor of just exporting an ES2015 Module :)

Issues

  • closes all Issues prior to html-loader <= v1.0.0

TODO

Status

Name Status
Default
HTML URLs
HMTL Imports
HMTL Templates
HTML Minification

Blocked by

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good!

@joshwiens
Copy link
Member

@michael-ciniawsky - Don't worry about splitting things in this PR.

Just edit the initial comment in #155 and add the appropriate Breaking Changes & we can handle the release notes portion there.

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

This is fine for now. We will all have to take another look at everything in next with any other v4 related updates

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Dec 14, 2017

I rebase the commit into CHANGELOG suiteable commits, revisit this one more time and land it on next during the day then ? So folks can test it out and I add the filters for options.url && options.import in follow up PR's ? Especially the current default url filter is a bit too aggressive :D

@d3viant0ne What is required to enable to CI e.g ok/sufficient to change the travis.yml to remove the master only stuff there (making a note to add it back, once this may land on master of course 🙃)? Or how should we proceed at best ?

@joshwiens
Copy link
Member

joshwiens commented Dec 14, 2017

- master

Just need to add - next to the branches list

Given next is used by Webpack already & something we are going to adopt, it should be in the branches list.

That list is actually necessary, it prevents Travis from running twice on pull requests.

@joshwiens
Copy link
Member

@michael-ciniawsky - We can land it now if you want. Once we are reasonably sure about next after this lands, we can throw out an initial .alpha-0 build to get people beating on it.

@michael-ciniawsky
Copy link
Member Author

I have one cricital bug fix locally and e2e tests (JSDOM), just a few minutes (~30) 😛 for final self review and to split the commits.

@joshwiens
Copy link
Member

@michael-ciniawsky - When you are satisfied with this, just merge it into next. I'll apply defaults 2.0 to that branch once it's finished & we can start getting alpha builds out.

@michael-ciniawsky
Copy link
Member Author

@d3viant0ne I just saw your comment I can leave the defaults v1.6.0...2.0.0 out of this PR if you want I test it out for myself in e.g style-loader, css-loader instead. Should I leave it here ?

@joshwiens
Copy link
Member

We can do it as a separate PR or you can update in this one. I suppose for the sake of keeping the commits in next as simple as possible for review, it may be easier to do the defaults upgrade as a separate pull request given the amount of change already in this one.

@michael-ciniawsky
Copy link
Member Author

kk I will leave it and open an issue alongside with the others I open for other contribs and as remaining TODO list, finally spit the commit here, land this PR in the next few moments (one final CI run) and then on to the CSS related stuff... 😛

@codecov
Copy link

codecov bot commented Dec 15, 2017

Codecov Report

❗ No coverage uploaded for pull request base (next@acda6f9). Click here to learn what that means.
The diff coverage is 98.92%.

Impacted file tree graph

@@           Coverage Diff           @@
##             next     #157   +/-   ##
=======================================
  Coverage        ?   97.89%           
=======================================
  Files           ?        5           
  Lines           ?       95           
  Branches        ?       32           
=======================================
  Hits            ?       93           
  Misses          ?        2           
  Partials        ?        0
Impacted Files Coverage Δ
src/lib/plugins/import.js 100% <100%> (ø)
src/lib/Error.js 100% <100%> (ø)
src/lib/plugins/url.js 100% <100%> (ø)
src/index.js 97.5% <97.43%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acda6f9...fea918f. Read the comment docs.

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Dec 15, 2017

Circle CI is expected to fail for this PR due to the missing setup (webpack-defaults@next)

The webpack Canary failure of the e2e tests is related to file-loader #234

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants