-
Notifications
You must be signed in to change notification settings - Fork 615
add support for ESM #4590
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
add support for ESM #4590
Conversation
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited that you're working on this. Some comments re: uniqueness issues that might or might not still be a real thing.
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited that you're working on this. Some comments re: uniqueness issues that might or might not still be a real thing.
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited that you're working on this. Some comments re: uniqueness issues that might or might not still be a real thing.
…505/modal-priority
…505/modal-priority
…505/modal-priority
…505/modal-priority
…505/modal-priority
…505/modal-priority
* upstream/main: (77 commits) PRO-6578: auto import of inline images (apostrophecms#4723) Inline array improvements (apostrophecms#4671) take credit for the fix as well as the "change" (apostrophecms#4725) PRO-6576 fix default values for object fields in widgets and generally unify newInstance implementations (apostrophecms#4722) release 4.7.0 (apostrophecms#4717) allows to render widgets in published mode (for non localized with pu… (apostrophecms#4709) fix subfield imports (it was a bug in fetchRelationships: false) (apostrophecms#4715) Add replaces configuration for editor context menus (apostrophecms#4714) bump uploadfs dep (apostrophecms#4712) add batch operations to pages (apostrophecms#4684) PRO-6477 undhandled promise errors (apostrophecms#4700) fix slat order (apostrophecms#4710) make apostoggle accessible (apostrophecms#4708) Improve context menu positions (apostrophecms#4706) remove focus blockers in page relationship editor (apostrophecms#4702) Release 4.6.1 mergeback (apostrophecms#4703) remove z-index bump on area schema fields, boost menu index (apostrophecms#4699) Pro 6345 fix deprecated sass (apostrophecms#4640) Keep widget focused when menu is open (apostrophecms#4695) skipReplace for changeDocIds (apostrophecms#4694) ...
…505/modal-priority
* upstream/main: Release 4.8.0 (apostrophecms#4742) Enable by default, other tweaks (apostrophecms#4741) PRO-6627: Fix array reorder and labels (apostrophecms#4740) Pro 6518 mobile preview (apostrophecms#4720)
* upstream/PRO-4505/modal-priority: Modify disabled prop (apostrophecms#4744) Fix media manager not handling search results properly (sometimes) flex media manager containers remove conditional rendering lint fix line wip split button focus, widget priority button make sure we have modal restore focus to last selected item after prop change remove logs focus considerations for media uploader switch promise timeout for nexttick retry wip wip
* upstream/main: Focus prioritized UI in on modal open (apostrophecms#4707) fix a rare race condition (apostrophecms#4755) prevent un-publishing singletons (apostrophecms#4739) PRO-6660: modules order (apostrophecms#4745) remove unused vue-template-compiler dependency (apostrophecms#4752) Fix pnpm: true (apostrophecms#4750)
* upstream/main: reverts fn that determines the appropriate priority ui in editor modals. awaits trapfocus (apostrophecms#4765) Hotfix changelog (apostrophecms#4758)
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, minor notes
* upstream/main: handle viewport values for breakpoint preview (apostrophecms#4748) Pro 6678 mobile preview dropdown (apostrophecms#4768) Fix menu focus (apostrophecms#4767) Update changelog for translations (apostrophecms#4766) [generate] Translate to German (apostrophecms#4759) [generate] Translate to Spanish (apostrophecms#4760) [generate] Translate to French (apostrophecms#4761) [generate] Translate to Italian (apostrophecms#4762) [generate] Translate to Portuguese (Brazil) (apostrophecms#4763) [generate] Translate to Slovak (apostrophecms#4764)
* upstream/main: Breakpoint clear (apostrophecms#4769) sass import warning (apostrophecms#4776) PRO-6657: Following values for dynamic choices (apostrophecms#4770) warnDev now prefixes in the expected way (apostrophecms#4746) Pro 6666 page move setting page as modified (apostrophecms#4771)
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying out ESM, and I tried the incremental path of renaming app.js to app.mjs and making it an ES module, and leaving everything else alone to start. That seems to work fine. The page manager comes up OK, for instance.
Then I tried changing modules/default-page/index.js to index.mjs and exporting its contents the ESM way. This fails, the page manager does not show anything and I get browser console errors. If I click New Page, I get a server console error.
Is this kind of incremental transition approach supported? It seems it would be hard to transition otherwise as people will have a mix of module versions etc.
My code is in incremental-esm in testbed. I start it manually with node app.mjs as I haven't touched package.json.
@apostrophecms/page: api-error: manager.newInstance is not a function
{
module: '@apostrophecms/page',
type: 'api-error',
severity: 'error',
url: '/api/v1/@apostrophecms/page?aposMode=draft&aposLocale=en',
path: '/api/v1/@apostrophecms/page',
method: 'POST',
ip: '::1',
query: { aposMode: 'draft', aposLocale: 'en' },
requestId: 'n3truoelseeqybl6xgwoun9t',
name: 'error',
status: 500,
stack: [
'at Object.newChild (/Users/boutell/apostrophecms/apostrophe/modules/@apostrophecms/page/index.js:1411:30)',
'at post (/Users/boutell/apostrophecms/apostrophe/modules/@apostrophecms/page/index.js:465:21)',
'at process.processTicksAndRejections (node:internal/process/task_queues:95:5)',
'at async /Users/boutell/apostrophecms/apostrophe/modules/@apostrophecms/module/index.js:191:30'
],
errorPath: undefined,
data: {}
}
boutell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bug is not a bug. We will document that you can't mix .js and .mjs, you must go all the way to "type": "module" and update your .js to ESM syntax.

Summary
Add ESM support
What are the specific steps to test this change?
npm testWhat kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: