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.

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

Jump to bottom

Preprocessing #31

Merged
merged 8 commits into from
Dec 17, 2017
Merged

Preprocessing #31

merged 8 commits into from
Dec 17, 2017

Conversation

esarbanis
Copy link
Contributor

Utilized svelte.preprocess introduced to svelte in v1.44.0

nikku
nikku suggested changes Dec 13, 2017
View reviewed changes
Copy link
Contributor

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Changes look good from my side apart from a few formating errors (we use tabs instead of spaces across JS files).

However, they seem to break the emitCSS option (cf. Travis). Is this something that was changed in Svelte > 1.0.6? If that is the case you'd need to adjust this test case, too.

index.js
@@ -31,8 +32,8 @@ module.exports = function(source, map) {

if (!options.name) options.name = capitalize(sanitize(options.filename));

try {
let { code, map, css, cssMap } = compile(source, options);
preprocess(source, options).then(processed => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use tabs instead of spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks!

test/loader.spec.js Outdated
resourcePath: 'test/fixtures/style-valid.html',
options,
},
fileContents,
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix formating; use tabs instead of spaces.

index.js Outdated
@@ -44,10 +45,10 @@ module.exports = function(source, map) {
utimesSync(tmpobj.name, stats.atimeMs - 9999, stats.mtimeMs - 9999);
}

this.callback(null, code, map);
Copy link
Contributor

Choose a reason for hiding this comment

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

Formating: use tabs instead of spaces.

@esarbanis
Copy link
Contributor Author

The weird thing is that I do have adapted the test cases to be async and they all pass in my local machine.

It could be some sort of permission limitation in travis, cause we write in a temp file, although /tmp should not need special permissions ... I really don't know what's going on with that test ...

@esarbanis esarbanis force-pushed the preprocessing branch 2 times, most recently from c287643 to 23fc3ab Compare December 14, 2017 09:58
@esarbanis
Copy link
Contributor Author

Found the issue in tests, in node 8.0.0 the Stats object does not contain an atimeMs or mtimeMs fields, so bumped node's version up to a more recent 8.x version

@esarbanis
Copy link
Contributor Author

esarbanis commented Dec 14, 2017 •
edited
Loading

Hm, now that I think of it we should better make the loader compatible with earlier versions of node as well.

I updated the change.

@nikku
Copy link
Contributor

nikku commented Dec 14, 2017 •
edited
Loading

Hm, now that I think of it we should better make the loader compatible with earlier versions of node.

Please ensure that we test this on travis, too. Just add the additional node versions you'd like to test to .travis.yml.

In my opinion testing stable and lts would be sufficient (cf. Travis docs).

@esarbanis
Copy link
Contributor Author

On it!

@esarbanis
Copy link
Contributor Author

esarbanis commented Dec 14, 2017 •
edited
Loading

According to this we should cover 6.x, 8.x and latest 9. The problem with the previous error was that from 8.0 the api changed, within the 8.x range. Anyway, I suggest the following:

node_js:
- '9' <-- latest
- '8' <-- latest 8.x
- '8.0' <-- exact
- '6' <-- latest 6.x

@esarbanis
Copy link
Contributor Author

esarbanis commented Dec 14, 2017 •
edited
Loading

OK, putting 8.0 is just dump as we cover the api changes from 6.x to 9.x! I removed it.

@Rich-Harris Rich-Harris merged commit 8b777f6 into sveltejs:master Dec 17, 2017
@Rich-Harris
Copy link
Member

Thank you! Released as 2.3.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@esarbanis @nikku @Rich-Harris