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

CJS compatibility dropped for @glimmer/syntax - intentional? #1668

Closed
davidtaylorhq opened this issue Nov 25, 2024 · 15 comments · Fixed by #1670
Closed

CJS compatibility dropped for @glimmer/syntax - intentional? #1668

davidtaylorhq opened this issue Nov 25, 2024 · 15 comments · Fixed by #1670

Comments

@davidtaylorhq
Copy link

Since 57e59c4, @glimmer/syntax is no longer including cjs modules in the published package. Was this an intentional breaking change?

For context: we use @glimmer/syntax directly to power a custom babel plugin, which is then consumed by ember-cli/embroider, which seem to only support cjs babel plugins.

@jakebailey
Copy link

The dropping of CJS stuff has also broken DefinitelyTyped's @types/ember__helper package, which depends on @glimmer/manager and @glimmer/runtime. Though, @ember/helper is confusingly internal so it's hard to say if that's a problem or not. (Certainly a problem for keeping DT green.)

@NullVoxPopuli
Copy link
Contributor

the DT types should use older copies of the glimmer dependencies -- we can't support DT types forever -- and I'm not actually sure even if we support them now -- feels kinda like a "Best effort" thing.

@glimmer/syntax absolutely should have a CJS option for stuff like what @davidtaylorhq mentioned, but also prettier.

I'll PR the re-adding CJS for just @glimmer/syntax.
We don't want to support CJS for all the packages though, because CJS is a huge pain, and since the VM is primarily something for browsers, it doesn't make much sense to emit CJS.

@mansona
Copy link

mansona commented Nov 27, 2024

@davidtaylorhq for the record Embroider main (i.e. vite/unstable) will support esm plugins embroider-build/embroider#2178

That PR even added a regression test so we won't backslide

@jakebailey
Copy link

I can prevent the DT package from pulling in 0.93 to get CI green, but I am a little confused as I thought glimmer and ember were under the same umbrella. I don't have enough context to understand what the private package @ember/helpers needs (and I don't really know why it was added to DT at all).

@NullVoxPopuli
Copy link
Contributor

The DT packages are not under ember really -- moreso a community best effort 💪
internal to ember, it depends on a bunch of things from the glimmer packages, so the DT types try to do the same (which is the better thing to do, usually) -- but the glimmer packages currently don't have semver guarantees due to all mostly being privateish packages

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 27, 2024

Just got around to poking at this, and it looks like CJS is already bundled:

image
image

🤔

Local testing does say that node is resolving the esm though.

@jakebailey
Copy link

Yeah, it's just locked off in a development condition which won't be applied by anyone; the same is true for the packages I mentioned. I'm not sure what the difference is, though, to know everything would be solved by pointing CJS to that dir unconditionally. (Or, maybe shipping the dev dir is a mistake)

@NullVoxPopuli
Copy link
Contributor

well, where this is annoying is that ember uses require to try to point at ESM. but... ember is probably in the wrong here -- and it had some build improvements recently, so if it's not already fixed, it's probably something that I need to update in ember so that problem stops coming up 🙈

@NullVoxPopuli
Copy link
Contributor

looks like only 3 deps need updating:

  • @glimmer/syntax
  • @glimmer/util
  • @glimmer/wire-format

@jakebailey
Copy link

FWIW I sent DefinitelyTyped/DefinitelyTyped#71275, but then found https://github.com/emberjs/ember.js/blob/main/packages/%40ember/helper/package.json?rgh-link-date=2024-11-27T18%3A43%3A53Z which actually is a module, but is also in TS, so I am in general, confused as to what's going on with all of this (I don't understand why any of this was on DT as it's private, why ember isn't at least pushing its types over to DT or something to match with the private package)

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 27, 2024

well, maybe the situation will improve in that DT PR after: #1669 is released

which actually is a module,

sort of secretly, most things in the ember ecosystem are sort of not, because none of the packages have type=module -- they used to be ambiguous, but most modern tooling treat js as cjs when type=module isn't set in package.json

@jakebailey
Copy link

It wouldn't improve unless manager and runtime are also changed; it does seem like this could be applied across all of the packages?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 27, 2024

manager and runtime are not used in any cjs tooling.

for compatibility with DT, your tsconfig (and their tsconfigs) should probably try using moduleResolution bundler, or node16 or something like that (please let me know if this is a terrible idea 🙈 )

@jakebailey
Copy link

for compatibility with DT, your tsconfig (and their tsconfigs) should probably try using moduleResolution bundler, or node16 or something like that (please let me know if this is a terrible idea 🙈 )

bundler is an illegal option on DT, but we have been considering allowing it for packages that should only ever be bundled.

node16 would work, but only if i make @ember/helpers be "type": "module", in which case I don't need glimmer to change anything at all. That actually seems right, given the package.json I linked to says "type": "module", it's just that I didn't find it until just now. The disconnect between DT and ember here is a challenge.

The main thing I'm not sure of is the version number this ember package is supposed to have, since there is no npm entry to tell me what the current version is to then put on DT.

@NullVoxPopuli
Copy link
Contributor

Turns out this @glimmer/syntax CJS oopsie will break our own tooling, too: https://github.com/emberjs/ember-test-waiters/actions/runs/12059491328/job/33628220301?pr=476#step:4:25

NullVoxPopuli added a commit that referenced this issue Nov 27, 2024
…tput

Fix #1668: Node does not use the 'development' condition with default require() - this PR removes the `development` condition under `require` for `@glimmer/syntax` and dependents.
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 a pull request may close this issue.

4 participants