Add asset pipeline for CSS #49

kaishin · 2020-04-04T11:55:32Z

Closes #44

Notable decisions:

Using Parcel v2 alpha, since v1 was ridden with CSS-related bugs. Parcel community seems to have given up on v1 for PostCSS.
Source files placed under ./Sources/web/css. Hesitated with folder name casing...
Output files are in ./Assets

Pending questions:

From the issue:

Alternatively, we can just copy the files into the built documentation directory, and have our HTML point to where they'll be. Inlining CSS was something I did primarily to get beta 1 out the door.

The swift-doc utility needs to know the path of the CSS file to copy over inside of run(), is that something we can do today with the current SPM limitations? Perhaps I am missing something.

If we use an external CSS path, we need to adapt the import path depending on whether the page is Home or a subpage.

mattt · 2020-04-04T13:31:10Z

This is great, @kaishin! Thanks for taking this on.

By the way, feel free to develop on a swift-doc branch instead of a fork. That'll make it easier for folks to collaborate on new features.

To that end, I just added a small patch to manage generated files according to this great article by @clarkbw. It should go a long way to cutting down on diff churn during code review.

Trying to get this running locally, I'm hitting a runtime error:

$ npm run watch

> swift-doc-assets@1.0.0 watch /Users/mattt/Code/SwiftDoc/swift-doc
> parcel watch Sources/web/css/**/*.css

/Users/mattt/Code/SwiftDoc/swift-doc/node_modules/@parcel/core/lib/serializer.js:28
    throw new Error('Name already registered with serializer');
    ^

Error: Name already registered with serializer
    at registerSerializableClass (/Users/mattt/Code/SwiftDoc/swift-doc/node_modules/@parcel/core/lib/serializer.js:28:11)
    at Object.<anonymous> (/Users/mattt/Code/SwiftDoc/swift-doc/node_modules/@parcel/package-manager/node_modules/@parcel/fs/lib/NodeFS.js:129:37)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10)
    at Module.load (internal/modules/cjs/loader.js:996:32)
    at Function.Module._load (internal/modules/cjs/loader.js:896:14)
    at Module.require (internal/modules/cjs/loader.js:1036:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (/Users/mattt/Code/SwiftDoc/swift-doc/node_modules/@parcel/package-manager/node_modules/@parcel/fs/lib/index.js:28:15)
    at Module._compile (internal/modules/cjs/loader.js:1147:30)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! swift-doc-assets@1.0.0 watch: `parcel watch Sources/web/css/**/*.css`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the swift-doc-assets@1.0.0 watch script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/mattt/.npm/_logs/2020-04-04T13_29_18_624Z-debug.log

Any ideas about what might be causing this?
Some additional details:

$ npm --version
6.14.4
$ node --version
v13.12.0

mattt · 2020-04-04T13:35:31Z

The swift-doc utility needs to know the path of the CSS path to copy over during run(), is that something we can do today with the current SPM limitations?

Unfortunately, no. As far as I can tell, this will require package resources, coming in Swift 5.3. Though, if don't mind taking an insecure, unsustainable approach in the interim, we could download the CSS asset over the web during document generation.

kaishin · 2020-04-04T13:59:36Z

Thanks for adding .gitattributes!

Trying to get this running locally, I'm hitting a runtime error

I will look into this. I personally use pnpm so I haven't come across it.
Related: Xcode insists on showing node_modules. There is no way to change the name of that folder in node afaik. Do you know of any way to work around this in Xcode?

By the way, feel free to develop on a swift-doc branch instead of a fork. That'll make it easier for folks to collaborate on new features.

My preference too. I tried but couldn't actually. Mind looking into it?

[...] we could download the CSS asset over the web during document generation.

I thought of that as well. How about we link to a remote CSS file instead? It's not the best approach, but it's an alternative to consider.

Another option would be to specify a CSS path argument when running the command and defaulting to the remote file otherwise (would even come with the benefit of opening the door to custom styling).

kaishin · 2020-04-06T00:06:54Z

The last commit should fix the issue. Remove node_modules and .cache and run npm install again. If you still get the error, run npm install parcel@next as a last resort. Issue: parcel-bundler/parcel#4064

mattt · 2020-04-06T12:33:45Z

Nice! Pulling the most recent commit and re-running npm install got this working for me.

By the way — I just updated the repository settings, so you should be able to create branches as a team member for future feature work.

mattt · 2020-04-06T13:25:27Z

Now onto some bikeshedding...

@kaishin What do you think about this layout?

.
├── Assets
│   ├── css
│   ├── images
│   └── js
├── Resources
│   ├── all.css
│   └── application.js
└── Sources
    ├── swift-doc
    └── (etc.)

So, all web stuff goes into Assets which then get compiled into Resources. That way, we keep Sources as Swift-only (no chance of confusing SPM) and will set ourselves up for package resources when that lands in Swift 5.3. 👍 / 👎 ?

kaishin · 2020-04-06T13:30:49Z

How about renaming Resources to Static? It’s not obvious at first glance what’s compiled into what... I think Resources/Static works as well.

kaishin · 2020-04-06T13:31:39Z

Oh I guess that’s already decided in SPM? In that case no objections.

mattt · 2020-04-06T15:11:31Z

Oh I guess that’s already decided in SPM? In that case no objections.

Looking at the accepted proposal, the feature doesn't make any assumptions about where resources are located. But following the convention of a target's sources being located in the Sources directory, it would make sense for its resources to be in Resources.

Regarding Static, I don't see enough benefit to overcome the inertia of the default Resources, but I could be convinced otherwise, if you feel strongly about it.

README.md

package.json

.gitignore

kaishin · 2020-04-06T15:59:12Z

@mattt Any thoughts about this? This PR is still a draft because it broke functionality that it hasn't restored yet.

How about we link to a remote CSS file instead? It's not the best approach, but it's an alternative to consider.

Another option would be to specify a CSS path argument when running the command and defaulting to the remote file otherwise (would even come with the benefit of opening the door to custom styling).

mattt · 2020-04-08T16:35:46Z

@kaishin

Any thoughts about this? This PR is still a draft because it broke functionality that it hasn't restored yet.

Apologies for not responding sooner. I've been heads-down the last few days working on 1.0.0-beta.2 (I wanted to cut a new release before all of this lands).

How about we link to a remote CSS file instead? It's not the best approach, but it's an alternative to consider.

My first thought was to write that off as hacky, but I think there's a lot of merit to this approach — especially in light of your point about it providing a customization point for end users. I'm concerned about network access being limited in certain environments, and the security implications of downloading assets, but I think we can reach a good compromise by doing the following:

Add support for Swift 5.3 development snapshots that support package resources by way of a Package.swift@5.3 file.
If a stylesheet can be loaded from the bundle, we'll use that. Otherwise, one will be downloaded from a remote URL and its contents verified against a SHA-256 digest.
We can use the same approach for any JavaScript that we add to the project.
In a future version, a user may be allowed pass an optional stylesheet-url / stylesheet-checksum options to use a different stylesheet.

mattt · 2020-04-08T16:56:18Z

@kaishin Also, curious to know how you've setup your editor to work with PostCSS. The only real change I want from standard SCSS-style rule nesting. Would it make more sense to formally adopt SCSS syntax with postcss-scss or use the postcss-nested to postprocessor? Have you found a reasonable configuration that allows for save-on-format and linting in VSCode?

kaishin · 2020-04-08T19:13:28Z

@kaishin Also, curious to know how you've setup your editor to work with PostCSS.

I can share those once I am back to my computer, but it’s nothing fancy. I use format-on-save and open all CSS files as PostCSS.

The only real change I want from standard SCSS-style rule nesting. Would it make more sense to formally adopt SCSS syntax with postcss-scss or use the postcss-nested to postprocessor?

This PR uses the nesting syntax that comes with postcss-presets-env, which is an implementation of this draft spec. I personally would recommend using this syntax instead of the SCSS, but changing it would take 5 minutes tops.

mattt · 2020-04-08T19:32:25Z

I can share those once I am back to my computer, but it’s nothing fancy. I use format-on-save and open all CSS files as PostCSS.

The marketplace has a bunch of VSCode extensions that are entirely indistinguishable to me, and none of them do a great job explaining exactly what combination of plugins they support. So any guidance here would be really helpful — something to document, even.

kaishin · 2020-04-09T21:52:17Z

@mattt Here is my VS Code setup for CSS/PostCSS!

postcss-sugarss-language: Syntax highlighting for PostCSS. Still maintained unlike the other alternatives.
Prettier Now: Provides format-on-save for JS, CSS, and PostCSS.
styelint Extension and .stylelint file for linting rules for CSS and PostCSS. I have a global one but sometimes introduce per-project overrides.

I have other things for colors and whatnot but these 3 are the main ones I rely on for CSS.

kaishin · 2020-04-09T21:55:58Z

Since I don't expect others to use pnpm, I will ignore the lock file for now.

Closes SwiftDocOrg#44

Mark Assets/index.css and package-lock.json as generated files, and treat them as binary for the purposes of diffing and merging.

Updated for Node and VS Code

kaishin · 2020-04-09T22:35:09Z

@mattt For the SHA256 digest, were you thinking of pulling swift-crypto as a dependency? I can do that in this PR, or we can leave it to a separate one.

kaishin · 2020-04-09T23:28:11Z

I added code for downloading and writing the CSS file—pointing to the fork URL for now. I can also preemptively add the final URL from master and update accordingly if we don't want to leave that for another PR.

Couple of annoyances:

Opening the HTML file without an http server fails to load the local CSS file.
Xcode development experience is ruined with node_modules. Let's discuss options.

kaishin · 2020-04-09T23:30:50Z

Sources/swift-doc/Supporting Types/Page.swift

@@ -43,3 +41,11 @@ func path(for symbol: Symbol) -> String {
 func path(for identifier: CustomStringConvertible) -> String {
    return "\(identifier)".replacingOccurrences(of: ".", with: "_")
 }
+
+func writeFile(_ data: Data, to url: URL) throws {


Perhaps writeFile(with data... would have been better. Or write(_ data but that one gave me some errors I wasn't able to track.

mattt · 2020-04-10T19:34:01Z

@mattt For the SHA256 digest, were you thinking of pulling swift-crypto as a dependency?

Thinking about the size of that dependency, I'm starting to have second thoughts. I care quite a bit about security, but I know a lot of folks don't (no judgment here — I get it). So I worry about adding a heavy dependency to do one thing that most people see as optional.

💡 Idea

It occurred to me that our downloading a CSS file and verifying its content is duplicating what a browser does with SRI for a <link> tag. So what if we computed that as part of he asset pipeline?

I found a WebPack plugin that generates an asset manifest with SRI digests (I don't know if Parcel has something similar, but I imagine that we could write a plugin ourselves as needed). When loading resources locally (including, eventually, JS), we could read the SRI digests from the JSON file and plug those right into the markup — no swift-crypto required.

But that's getting ahead of myself.

To get this PR merged, how about we do this:

Forget SRI / checksum validation for now, and just link to a static file hosted in our repo
In a new PR, we can add a --stylesheet-URL option, which defaults to this.
After that, we could add a --stylesheet-subresource-integrity option, too, to accommodate that use case.
Finally, for Swift 5.3, we can add support for loading a local stylesheet.

I added code for downloading and writing the CSS file—pointing to the fork URL for now. I can also preemptively add the final URL from master and update accordingly if we don't want to leave that for another PR.

Yes, let's do that to avoid the extra follow-up PR.

Couple of annoyances:

Opening the HTML file without an http server fails to load the local CSS file.

Related: #65 (comment)

Xcode development experience is ruined with node_modules. Let's discuss options.

I think this can be solved by generating and committing an Xcode project that removes node_modules (一石二鳥, this would also fix #60).

kaishin · 2020-04-11T22:51:07Z

Thinking about the size of that dependency, I'm starting to have second thoughts. I care quite a bit about security, but I know a lot of folks don't (no judgment here — I get it). So I worry about adding a heavy dependency to do one thing that most people see as optional.

Indeed. I wasn't quite sold on the return on investment in this particular instance, not until we introduce JavaScript at least, where the security concerns are many-folds more serious than CSS.

I am more than happy to look into SRI after this PR.

I think this can be solved by generating and committing an Xcode project that removes node_modules (一石二鳥, this would also fix #60).

了解！

For the other items you listed, it'd be easier to have issues for them. I can create them if you're ok with it.

Remove name and version field from package.json

mattt · 2020-04-16T20:30:57Z

I think this can be solved by generating and committing an Xcode project that removes node_modules (一石二鳥, this would also fix #60).

Turns out, the generated project doesn't link libxml2 correctly. Also, I'm pretty sure that we'd need to regenerate the file whenever a dependency is added or removed.

I posted a message on the Swift Forums about the Xcode issue here: https://forums.swift.org/t/hiding-ignoring-directories-from-xcode-when-opening-swift-packages/35431

How bad to you think the current setup is? Do you consider this a blocker?

For the other items you listed, it'd be easier to have issues for them. I can create them if you're ok with it.

That would be great, thanks!

kaishin · 2020-04-16T21:48:03Z

How bad to you think the current setup is? Do you consider this a blocker?

If generate-xcodeproj doesn't solve the issue, I'd like to pitch using yarn exclusively for this project, making use of .yarnrc and --modules-folder and moving the modules folder to .node-modules. It's a lost cause with npm, and frankly messing with NODE_PATH env variable is asking for trouble since it wasn't designed for this use.

I've switched to VS Code for this project (and other non-Apple platform projects) and it's been good so far. But it's worthwhile to do this work so as to not impact the Xcode development experience.

Give me 👍 or 👎 so that I can go forward with the yarn solution. Of all the ones I've thought about, it's the cleanest.

mattt · 2020-04-17T00:01:41Z

@kaishin Great idea! I'd like to continue using Xcode as my primary IDE for this project (and allow others to as well), so this is perfect.

kaishin · 2020-04-17T19:49:33Z

The last commit is quite a departure from what we discussed above. I'll elaborate.

The .yarnrc solution combined with the alpha release of parcel was a recipe for disaster. The --modules-folder flag itself is flimsy, and even after I got things to work, parcel was still failing to pick the PostCSS config when run from .node_modules. Which leads me to my next point.
Parcel 2.0 is still in alpha, and the documentation is pretty much non-existent outside the README. It's also a lot of magic, and these two don't mix well together.
Parcel 1.0 works, but as soon as I use the .node-modules trick things go haywire. It literally overwrites the package.json with some total nonsense.
I started considering WebPack, and even got it set up, but at this stage I'd say it's more hassle than it's worth. 5.0 is going to be released soon, so we can add it once that's done and once we need to start adding JS into the mix.

The solution:

I started by moving all node files to .node, that way it won't show in Xcode.
Since PostCSS is our only concern at this stage, I thought it'd be better to just use postcss-cli directly. It works like a charm, clear to use and configure, and does only one thing.
It's hard to see the diff of the CSS output when it's minified. For that I made the all.css the non-minified version generated when running watch and all.min.css the production version generated when running build.

kaishin · 2020-04-17T20:08:36Z

The file structure inside .node is as follows:

├── node_modules 
├── package-lock.json
├── package.json
└── postcss.config.js

mattt · 2020-04-18T13:21:32Z

@kaishin That sounds like quite the ordeal. I can't thank you enough for braving the trash fire that is the JavaScript ecosystem to make this asset pipeline happen.

Since your last commit, I made a small edit to the Development section of the README and added a changelog entry. (I also applied a fast-follow patch with 7705c4f to address some differences with the original CSS)

My next steps will be to take the original source and split it across a few different files, and looking into generating inlined, Base64-encoded SVG images. Also, experimenting with package resources in the latest Swift 5.3 preview.

kaishin · 2020-04-18T13:46:54Z

@mattt Don't mention it. Thanks for your patience, as well. 😉 If you break things down into tasks I'd be happy to pick some of them up, including splitting up the CSS, Base64-encoded SVG images, etc.

For postcss-nested, it might be worthwhile to turn off postcss-preset-env's own nesting rules to avoid unnecessary work and potential hassle when debugging.

mattt reviewed Apr 6, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

mattt reviewed Apr 6, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

mattt reviewed Apr 6, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

kaishin and others added 7 commits April 10, 2020 00:09

Add asset pipeline for CSS

94f3c12

Closes SwiftDocOrg#44

Add .gitattributes file

6ec5a37

Mark Assets/index.css and package-lock.json as generated files, and treat them as binary for the purposes of diffing and merging.

Re-run npm to update dependencies

df81063

Change file structure and clean up package.json

adbec56

Update .gitignore using github/gitignore

4bd139a

Updated for Node and VS Code

Rename index.css to all.css

fd60f8e

Ignore pnpm artifact

f03b6b9

Add CSS file download

a4ecfb4

kaishin force-pushed the reda/css-build branch from 7f6580e to a4ecfb4 Compare April 9, 2020 23:24

kaishin marked this pull request as ready for review April 9, 2020 23:24

kaishin commented Apr 9, 2020

View reviewed changes

Update CSS file URL

9b309ba

kaishin and others added 4 commits April 12, 2020 00:51

Merge branch 'master' into reda/css-build

083b37a

Merge branch 'master' into reda/css-build

c77fc44

Remove pnpm-lock.yaml

d9dcba8

Add private field to package.json

cc55796

Remove name and version field from package.json

Use post-css cli and move node files to .node

6587f53

kaishin and others added 5 commits April 17, 2020 22:11

Update .gitattributes

ee4565a

Update Development section in README.md

d5ad804

Update Assets

ef1d0b2

Formatting

b12b682

Add changelog entry for asset pipeline

b09ffc0

mattt merged commit 4a33d76 into SwiftDocOrg:master Apr 18, 2020

mattt mentioned this pull request Apr 18, 2020

Update Asset Pipeline Configuration #84

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add asset pipeline for CSS #49

Add asset pipeline for CSS #49

kaishin commented Apr 4, 2020 •

edited

Loading

mattt commented Apr 4, 2020

mattt commented Apr 4, 2020

kaishin commented Apr 4, 2020 •

edited

Loading

kaishin commented Apr 6, 2020

mattt commented Apr 6, 2020

mattt commented Apr 6, 2020

kaishin commented Apr 6, 2020

kaishin commented Apr 6, 2020

mattt commented Apr 6, 2020

kaishin commented Apr 6, 2020 •

edited

Loading

mattt commented Apr 8, 2020 •

edited

Loading

mattt commented Apr 8, 2020

kaishin commented Apr 8, 2020

mattt commented Apr 8, 2020 •

edited

Loading

kaishin commented Apr 9, 2020 •

edited

Loading

kaishin commented Apr 9, 2020

kaishin commented Apr 9, 2020

kaishin commented Apr 9, 2020 •

edited

Loading

kaishin Apr 9, 2020

mattt commented Apr 10, 2020 •

edited

Loading

kaishin commented Apr 11, 2020 •

edited

Loading

mattt commented Apr 16, 2020

kaishin commented Apr 16, 2020

mattt commented Apr 17, 2020

kaishin commented Apr 17, 2020 •

edited

Loading

kaishin commented Apr 17, 2020

mattt commented Apr 18, 2020

kaishin commented Apr 18, 2020

Add asset pipeline for CSS #49

Add asset pipeline for CSS #49

Conversation

kaishin commented Apr 4, 2020 • edited Loading

mattt commented Apr 4, 2020

mattt commented Apr 4, 2020

kaishin commented Apr 4, 2020 • edited Loading

kaishin commented Apr 6, 2020

mattt commented Apr 6, 2020

mattt commented Apr 6, 2020

kaishin commented Apr 6, 2020

kaishin commented Apr 6, 2020

mattt commented Apr 6, 2020

kaishin commented Apr 6, 2020 • edited Loading

mattt commented Apr 8, 2020 • edited Loading

mattt commented Apr 8, 2020

kaishin commented Apr 8, 2020

mattt commented Apr 8, 2020 • edited Loading

kaishin commented Apr 9, 2020 • edited Loading

kaishin commented Apr 9, 2020

kaishin commented Apr 9, 2020

kaishin commented Apr 9, 2020 • edited Loading

kaishin Apr 9, 2020

Choose a reason for hiding this comment

mattt commented Apr 10, 2020 • edited Loading

kaishin commented Apr 11, 2020 • edited Loading

mattt commented Apr 16, 2020

kaishin commented Apr 16, 2020

mattt commented Apr 17, 2020

kaishin commented Apr 17, 2020 • edited Loading

kaishin commented Apr 17, 2020

mattt commented Apr 18, 2020

kaishin commented Apr 18, 2020

kaishin commented Apr 4, 2020 •

edited

Loading

kaishin commented Apr 4, 2020 •

edited

Loading

kaishin commented Apr 6, 2020 •

edited

Loading

mattt commented Apr 8, 2020 •

edited

Loading

mattt commented Apr 8, 2020 •

edited

Loading

kaishin commented Apr 9, 2020 •

edited

Loading

kaishin commented Apr 9, 2020 •

edited

Loading

mattt commented Apr 10, 2020 •

edited

Loading

kaishin commented Apr 11, 2020 •

edited

Loading

kaishin commented Apr 17, 2020 •

edited

Loading