feat: add support for the latest spec #126
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #126 +/- ##
==========================================
+ Coverage 92.06% 94.00% +1.93%
==========================================
Files 5 6 +1
Lines 189 200 +11
Branches 32 29 -3
==========================================
+ Hits 174 188 +14
- Misses 4 8 +4
+ Partials 11 4 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
LGTM Except in the imports. you directing the imports to an js files, it would be way better if the imports actually link to the typescript and not some build files, as this could allow a build free use in deno. Btw |
|
@lucsoft, you mean you'd like to have a |
| @@ -0,0 +1,37 @@ | |||
| import type ConstructedStyleSheet from './ConstructedStyleSheet.js'; | |||
| import type Location from './Location.js'; | |||
| import type { UnknownFunction } from './shared.js'; | |||
There was a problem hiding this comment.
kinda. you don't need to checkout the transpiled javascript files but like these imports:
| import type { UnknownFunction } from './shared.js'; | |
| import type { UnknownFunction } from './shared.ts'; |
Would be cool if its 1:1 imports
There was a problem hiding this comment.
Hey, sorry, I missed your comment for some reason.
If I understand correctly, you think I'm importing transpiled JavaScript files instead of directly importing TypeScript files. However, this is not the case. The .js extension is just an ESM convention, so I'm actually referring to the .ts file even though I'm using the .js extension. Also, according to the specification, another extension besides .js is not allowed (until the import attributes proposal is accepted).
There was a problem hiding this comment.
I'm coming from the Deno perspective, where there is no magic build folder.
so you import your file via import {} from './constants.ts' (how it actually is on the file system) (in nodejs+ts this would be done via allowImportingTsExtensions)
There was a problem hiding this comment.
I guess I got it. So all I need is to have additional adoptedStyleSheets.ts in the build folder to support Deno. I was kinda confused by your examples of my code 😅 Didn't get that it was an example.
Ok, I'll add it when I have time to fix this PR.
There was a problem hiding this comment.
no in deno you don't need a build folder i would like to just import it via the github raw cdn
so i just import the stuff form src
thats what i mean with using allowImportingTsExtensions so it actually is in the src folder correct not after the build
:D hope i made it clear
|
@calebdwilliams any updates when this will land? |
|
Yikes I’ve been missing emails on this. I’ll review today and push. |
|
@Lodin tests seem to be failing on MacOS. Code looks mostly fine but I didn't do a deep dive due to the tests. Once that's complete, I'd be happy to merge. |
|
Thanks for the review. It's a bit hard for me to find time to finalize the PR but I'll try to do it ASAP. |
Resolves #124.
This PR adds support for the latest version of the specification (
Proxy(Array)instead ofFrozenArray).I have also done a refactoring to put all the code in the regular classes; they are transformed by Babel during the rollup build.
To reduce the common size, I used
tersercompilation, so the resulting file is minified. According tosize-limit, the size of the package now is2.67 Kb, and the overall approach looks better than before.