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

Shorten CSS references for @import or @use #1332

Merged
merged 4 commits into from
Sep 6, 2021

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Aug 17, 2021

For our own references, we will only use the name as relative to /ts/sass, e.g. ts/sass/button-mixins becomes button-mixins.

For references to npm modules, we will use the path relative to node_modules, e.g. ts/sass/bootstrap/alert becomes bootstrap/scss/alert.

This will make it easier for an external user to use our Svelte components. They would need to link our sass library under /ts/sass in their includePaths, and any of the libraries we use internally (so far bootstrap and codemirror).

@dae
Copy link
Member

dae commented Aug 18, 2021

I've fixed the spurious error, but that's revealed another. SASS_PATH in svelte.bzl:svelte_check() might need adjusting? I'll need to test this works in an external workspace too.

@hgiesel
Copy link
Contributor Author

hgiesel commented Aug 18, 2021

21b6696 contains what I thought might fix the issue, however it doesn't. I'm not quite sure why though. The runtime directory seems to contain all the necessary files.
Exporting //ts:sass from ts/BUILD.bazel and using that made the SCSS files resolve, but instead I got warnings that they shadow existing files.

@dae
Copy link
Member

dae commented Aug 19, 2021

This seems to fix it, which implies it was not working previously:

diff --git a/ts/svelte/svelte.bzl b/ts/svelte/svelte.bzl
index 565ad518f..3c87a21d2 100644
--- a/ts/svelte/svelte.bzl
+++ b/ts/svelte/svelte.bzl
@@ -93,7 +93,7 @@ def svelte_check(name = "svelte_check", srcs = []):
             "//ts/lib",
             "@npm//sass",
         ] + srcs,
-        env = {"SASS_PATH": "$(rootpath //ts:tsconfig.json)/../sass"},
+        env = {"SASS_PATH": "ts/sass"},
         # a lack of sandboxing on Windows breaks the local svelte_check
         # tests, so we need to disable them on Windows for now
         target_compatible_with = select({

The build seems to work correctly on an external workspace, and while I suspect svelte_check won't, that's not a big concern. Please let me know when you've pushed the change and I'll run the test on Windows to confirm it's ok there, and then it should be ok to merge once we get another bugfix release out the door.

@hgiesel
Copy link
Contributor Author

hgiesel commented Aug 19, 2021

Fix pushed.

@dae
Copy link
Member

dae commented Aug 19, 2021

Thanks, looks good on Windows too

@dae
Copy link
Member

dae commented Sep 2, 2021

Aside from having less text to type, are there any advantages to this PR after #1340? Paths relative to the repo root (eg ts/...) are standard in Bazel, so we'd be moving a bit away from common practice by doing this.

@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 2, 2021

When add-on devs use SCSS, they could reuse Ankis SCSS, which would be nice.

I've added a symlink anki in ts/sass which leads to ., this way add-on devs can use the SCSS like

@use 'anki/button-mixins' as button; 

Here's an example how I set up New Format Pack to use Ankis SCSS.

@dae
Copy link
Member

dae commented Sep 6, 2021

Thanks, appreciate the example. I was initially concerned about API issues, but changes to our sass shouldn't affect already-built add-ons, so that seems like it could work.

@dae dae merged commit 6909a09 into ankitects:main Sep 6, 2021
@dae
Copy link
Member

dae commented Sep 6, 2021

The symlink is causing a warning on all builds:

(11:19:25) ERROR: Infinite symlink expansion, for ts/sass/anki, skipping: Infinite symlink expansion

I'll remove it for now - can we accomplish the same thing a different way?

dae added a commit that referenced this pull request Sep 6, 2021
@hgiesel
Copy link
Contributor Author

hgiesel commented Sep 6, 2021

It's not really a big deal, as users can still reference our SCSS without the Anki prefix (which only becomes a problem, if there duplicate names), or recreate the symlinks themselves.
E.g. the add-on dev can create a scss/ directory himself, reference it in the includePaths, and put a symlink called anki in there that references the Anki sass/ directory.

As long as we don't publish an NPM module, the add-on dev will require an external reference, that will be machine dependent. Be it in the includePaths, or through a symlink in their project.

@hgiesel hgiesel deleted the svelteexportcontext branch September 6, 2021 12:17
dae added a commit that referenced this pull request Sep 13, 2021
Shorten CSS references for `@import` or `@use`
dae added a commit that referenced this pull request Sep 13, 2021
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