Skip to content

[#79] relative css injection improvements #78

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

Conversation

Codex-
Copy link
Contributor

@Codex- Codex- commented Feb 6, 2023

This is a bit chunky, i've abstracted a few chunks out that can be isolated and tested independently, but have not added any tests yet.

Have a play, and see what you think @marco-prontera

I imagine we'd want to move the abstracted methods into either utils or another file

@Codex- Codex- force-pushed the implement_relative_css_injection branch from 3fd8636 to 333c294 Compare February 6, 2023 23:31
@Codex- Codex- force-pushed the implement_relative_css_injection branch from 92c79ec to 4984ab0 Compare February 7, 2023 03:21
@Codex-
Copy link
Contributor Author

Codex- commented Feb 7, 2023

From here, just need to decide if we want the exported functions in their own file or with the other utils, I also found an oddity where this uses esbuild with an explicit target instead of using the one from the vite config, but can sort that in another PR :)

Copy link
Owner

@marco-prontera marco-prontera left a comment

Choose a reason for hiding this comment

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

First of all, thank you!
I wrote some comments that can improve this thing a little bit, and after that, I think we are ready!

@marco-prontera marco-prontera added the enhancement New feature or request label Feb 7, 2023
@marco-prontera marco-prontera self-assigned this Feb 7, 2023
@marco-prontera marco-prontera changed the title Implement relative css injection [#79] relative css injection improvements Feb 7, 2023
@marco-prontera marco-prontera linked an issue Feb 7, 2023 that may be closed by this pull request
@marco-prontera
Copy link
Owner

From here, just need to decide if we want the exported functions in their own file or with the other utils, I also found an oddity where this uses esbuild with an explicit target instead of using the one from the vite config, but can sort that in another PR :)

I opened an issue to track the thing about the 'build.target'!

I think we can write named functions instead of anonymous functions because it makes debugging more difficult. And moving all the functions from the top to the bottom of the file will make it a little more readable, do you agree?

@Codex-
Copy link
Contributor Author

Codex- commented Feb 7, 2023

I opened an issue to track the thing about the 'build.target'!

Excellent, should be an easy fix :)

And moving all the functions from the top to the bottom of the file will make it a little more readable, do you agree?

Can do, did you want to keep these new functions in the file though? The problem is with keeping them there is that they're now exposed as part of the public API, ideally you want to import them from another file without re-exporting them and then only exporting the plugin / interfaces right?

@Codex-
Copy link
Contributor Author

Codex- commented Feb 7, 2023

I think we can write named functions instead of anonymous functions because it makes debugging more difficult.

Which did you want to name? Any that are stored in a var actually return their name:

const someFunc = () => { throw new Error() };
> const someFunc = () => { throw new Error() };
undefined
> someFunc()
Uncaught Error
    at someFunc (REPL1:1:32)
> 

@Codex- Codex- marked this pull request as ready for review February 7, 2023 21:30
@marco-prontera
Copy link
Owner

I opened an issue to track the thing about the 'build.target'!

Excellent, should be an easy fix :)

And moving all the functions from the top to the bottom of the file will make it a little more readable, do you agree?

Can do, did you want to keep these new functions in the file though? The problem is with keeping them there is that they're now exposed as part of the public API, ideally you want to import them from another file without re-exporting them and then only exporting the plugin / interfaces right?

You are right, at this point, you can move also the functions that are now at the bottom also in the utils file.
For the anonymous functions question, I think it's better to give a name to functions used inside the filter and map calls (but it's not a problem)

@Codex- Codex- force-pushed the implement_relative_css_injection branch from f114145 to 3a05d23 Compare February 8, 2023 21:05
@Codex-
Copy link
Contributor Author

Codex- commented Feb 8, 2023

You are right, at this point, you can move also the functions that are now at the bottom also in the utils file. For the anonymous functions question, I think it's better to give a name to functions used inside the filter and map calls (but it's not a problem)

  • Moved the helpers to utils
  • Moved the tests to utils.spec
  • Reorganised the existing tests a little.
  • Updated some of the anonymous functions to be named.

@marco-prontera
Copy link
Owner

You are right, at this point, you can move also the functions that are now at the bottom also in the utils file. For the anonymous functions question, I think it's better to give a name to functions used inside the filter and map calls (but it's not a problem)

  • Moved the helpers to utils
  • Moved the tests to utils.spec
  • Reorganised the existing tests a little.
  • Updated some of the anonymous functions to be named.

Perfect!

@marco-prontera marco-prontera merged commit c45e7a4 into marco-prontera:feature/#76 Feb 8, 2023
marco-prontera added a commit that referenced this pull request Feb 8, 2023
* docs: fix documentation

* fix: make it work with legacy plugin

* refactor: readability

* style: cosmetics

* 2.5.0

* docs: improve documentation

* [#79] relative css injection improvements (#78)

* feat: explicitly handle Uint8Array to string coercion

* feat: abstract js asset collection to utility function

* feat: progress on relativeCSSInjection implementation

* feat: Throw same error on relative codepath if no assets are identified

* feat: Add additional debug logging.

* feat: Prevent double operation for css asset removal

* feat: Remove space-wrangling function.

- esbuild or the consuming vite bundling should handle this for minification if specified.

* feat: Move the type extension to interface

* test: extractCssAndDeleteFromBundle

* test: use narrowing to remove cast

* test: concatCss

* fix: make filtering usage more clear, simplify behaviour

* test: buildJsCssMap

* fix: use correct naming for chunk vs asset in map building

* test: test the metadata missing case

* test: getJsAssetTargets

* test: relativeCssInjection

* feat: move bundle deletion to the css reducer

* feat: log warning if some css assets remain in the bundle

* feat: move all helpers to util and merge test suites

* feat: change some anonymous functions to named

---------

Co-authored-by: Alex Miller <codex.nz@gmail.com>
@Codex- Codex- deleted the implement_relative_css_injection branch February 8, 2023 21:41
@Codex-
Copy link
Contributor Author

Codex- commented Feb 8, 2023

We should probably add something like:

#### relativeCSSInjection (boolean)

Enabling `relativeCSSInjection` will identify which JS assets import which CSS, and inject the imported CSS only to the directly related JS assets.

Any CSS assets not associated with a JS asset will be logged at the end of `relativeCSSInjection`, however, this can be suppressed by also setting `suppressUnusedCssWarning` to `true`.

to the docs for this 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

relative css injection improvements
2 participants