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

feat: support dsd #45

Merged
merged 5 commits into from
Jun 19, 2023
Merged

feat: support dsd #45

merged 5 commits into from
Jun 19, 2023

Conversation

bennypowers
Copy link
Collaborator

This PR updates the syntax of the built files to es2022, which includes ecmascript private fields and methods. By doing so, although the package will lose support for older browsers, the bundle sizes will be reduced because helpers like __privateGet will not be added.

In order to accomplish this, this PR implements a workaround for an open issue on esbuild with regards to ecma private fields.

evanw/esbuild#2800 (comment)

In addition, this PR adds support for Declarative Shadow DOM templates by checking for the presence of a shadow root before creating one.

src/slidem-slide-base.js Show resolved Hide resolved
build.js Outdated Show resolved Hide resolved
src/slidem-slide-base.js Show resolved Hide resolved
build.js Show resolved Hide resolved
src/slidem-deck.js Outdated Show resolved Hide resolved
@ruphin
Copy link
Owner

ruphin commented May 11, 2023

My suggestion for this PR is to handle each part separately.

Small fixes

Pull the two small fixes (optional chaining and shadowroot check) into the random fixes PR I have open, and merge all that basically right away.

Build tooling change

With regards to the build tooling change, I don't mind updating the build script, but I have some fundamental issues with updating the source (with the IIFEs) to optimize the build output for the short period where the build tool is bugged. My preference is to either wait until esbuild is fixed before merging these changes, or merging without the source change and accepting that the build is suboptimal until esbuild is fixed, and doing a patch release with updated build when the fix lands there.


We can work on pulling the styling out of slidem-slide-base in a separate PR/branch.

@bennypowers bennypowers changed the title feat: support dsd, es2022 feat: support dsd May 12, 2023
@bennypowers
Copy link
Collaborator Author

I removed the iifes. I don't like the mangled output, it makes debugging harder and there's a performance penalty at load and run time, but i also don't care enough to make the case

@bennypowers bennypowers requested a review from ruphin May 12, 2023 13:47
@bennypowers
Copy link
Collaborator Author

@ruphin is this good to merge now?

@ruphin
Copy link
Owner

ruphin commented May 15, 2023

Apologies, I was busy this weekend. I'll review today but I think we're good

@ruphin
Copy link
Owner

ruphin commented May 21, 2023

Looking at the build output there's two things that confuse me still. The first is that I don't understand where the vars and functions come from come from. The output style should be es2022, why doesn't it output const and () =>?

One more thing that's weird is that it injects the directory structure of my machine into the output: // css-constructible:/home/ruphin/dev/slidem/src/slidem-deck-global.css. Is that even relevant, considering that this file is not accessible since it's not bundled into the published package?


Thinking more about the build output, the original reasoning behind the build step was that so you could load and run this thing from CDNs like unpkg. It doesn't make much sense to build into es2022, which just ends up reducing compatibility.

In general, I'm not a huge fan of packages that don't ship source but instead ship some build output. I think the user of a package should decide how to build and what output target they want.

I could see changing the NPM package completely and just shipping the source directly. We could still have a build that produces output in /dist or something similar, that can be used on CDNs. Maybe a 3.0 release.

But let me first try to understand your reasoning behind changing the build to output es2022. You are the main consumer of this package, how do you consume it and what are your requirements in this regard? How do you prefer packages to be published in general?

@bennypowers
Copy link
Collaborator Author

The output style should be es2022

You asked me to undo the workaround for the esbuild bug which downgrades the output target

directory structure of my machine

I couldn't reproduce. what's your OS? perhaps we could adopt changesets and release from CI/CD

@bennypowers
Copy link
Collaborator Author

How do you prefer packages to be published in general?

I think es2022 is fine https://caniuse.com/mdn-javascript_classes_private_class_fields

@bennypowers
Copy link
Collaborator Author

Will try this again now that evanw/esbuild#3167

@bennypowers
Copy link
Collaborator Author

esbuild 0.18.4 solves the problem, the built output is now much closer to source

@ruphin
Copy link
Owner

ruphin commented Jun 19, 2023

Looks good to me. Not sure why it outputs var and function style code, but whatever I guess ¯\(ツ)

@ruphin ruphin merged commit fcf7b81 into master Jun 19, 2023
@bennypowers bennypowers deleted the es2022 branch June 19, 2023 06:17
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