fix: define es6 package as es6 package #5635
Conversation
|
Looks good to me. |
|
To remain consistent with standards: "cjs" - node only code written in commmonjs The package.json directs a few things for consumers of the library. There's a few of odd naming conventions and configuration settings in place to "make things work" because the package type was not set. I'd like to talk about build products and simplify this in another PR. |
|
Looks good to me too, even though I don't have the time right now to check every consequence... but that will come by itself. Wouldn't we need a About renaming every engine file to .mjs... I basically just like the idea because in some contexts you simply cannot quickly judge the content otherwise without peeking into the file (and even then it could be a wrong assumption) + e.g. file filtering in VSCode would be more precise. |
|
Does not look like a "package.json" is required for them. Only question mark is opentype and earcut in the scripts/textmesh folder, everything is either an IIFE (which works everywhere) or just simple javascript in a declarative es5 style (which works everywhere). If they are "node only commonjs", they should be named cjs to avoid bundling them for browsers by mistake. Using the correct application of "cjs", "mjs" and "js" keeps things very clear. Marking browser code as node code, in order to know what is es5 and es6 is odd. |
|
Update: finished checking |
|
Can you explain how you test the changes here? You talk about "tools", but which tool breaks and how do you notice that the breakage is fixed with the changes here - to make it obvious what we are dealing with here Since you define type=module in Lines 1 to 3 in 3032623 |
|
Happy to help, its a big topic there's a few perspectives so help me narrow this down. I'll write out my main thoughts and motivation. Yes, that is not required and can be removed as well. |
|
My approach to tool testing: I maintain public template projects on github for all the major bundlers under this account. (vite, webpack, parcel, variant configurations of rollup) I quickly spin them up to ensure a dependency can be consumed gracefully. I maintain a public template project specifically for playcanvas under this account to test how the library is consumed by 3rd parties. |
|
The Through the In that grid, this setting communicates to:
Through the This will be essential in divesting "extras" from core code and having a good developer experience. When its set correctly on each project dependency in your app, you don't think about what is cjs, es modules, iife, you just import and it works. The build tools in this project (and the majority of the popular build tools) actually transform everything to ES modules before processing. Sometimes just to transform them back, sometimes to ensure they align to the requirements of the build product. Setting it correctly leaves room for overrides by file extension. The overrides become redundant on a modern project, so, "mjs" and "cjs" can be used as a distinction between "browser" client code and "node" tooling code. Since only node needs the "cjs" extension, it gets earmarked with extensions. Sometimes its just nice to ignore the package.json to force a CJS import for constants or plain javascript for a mesh decimation tool. Requires extra steps if you lock in mjs. This project is less than an handful of of files away from living in a ES2022 world. Literally a weekend and 3 PRs. The only meaningful file extension for "source" after the next few weeks is "js" vs "ts". I agree, this has more meaning in projects which have more of a mixture of frontend, backend, and CLI. Seeing as some PC projects sometimes assume they are all cloned into the same parent folder as Engine, it's a valid concern. |
|
@epreston Thank you for your feedback, currently engine/examples/rollup.config.js Lines 147 to 151 in 3032623 That implies you cannot do Now every problem has a bunch of different solutions... and testing for
But yes, I also see your point... in a way I like to keep everything ret = await import("http://127.0.0.1/playcanvas-engine/build/playcanvas.js")Important difference is:
And this difference can then be used to check if it was ESM or not... in case of ESM, we have to do You talked about this article yesterday: https://2ality.com/2016/11/trace-globals-proxy.html And I'm not sure how that relates to importing or how that helps, because it is eval-ing a string, so it doesn't "hook" into the
Right, everything is confusing 😅 And iife is unified in umd, what the Anyway, I think this discussion was already fruitful and I will remove some dependence on MJS file extension checking in #5555 while not introducing more code... not sure if I can do so 100% yet. |
|
That's a check on build products. It's not related to this. No limitations are introduced. In a world where file extensions are omitted and people have more than a handful of dependencies in their projects, nobodies managing importmaps by hand to ship products. There is no need to support UMD for any platform or use case. It's the full technical equivalent of IIFE with less compatibility and no defined standard. Delivering an IIFE supports the oldest legacy and ESM in one shot. About the ES6 proxies, examples, editor: I'm writing up a discussion combining your ideas with what I see. |
no longer required
Reserve extensions for marking node code, not client code.
Update root file
|
I retract my LGTM after 738d19d I know your stance and all, but it doesn't make sense to me. I've attempted to explain the reasoning behind the JS/MJS change multiple times, but it appears you haven't quite understood yet or you simply don't want to see the issue. |
|
You want to use the most granular definition when it could be reserved, leaving space for it to be ignored. It's an odd measure aligned to managing importmaps by hand. |
|
My feeling is that we should consider parking the JS->MJS rename for now and allow @epreston to run with this for a bit. We can reassess the rename later - I mean, it was just a casual suggestion from me in the first place. Unless there's something blocking the merging of the rewritten Examples Browser? |
I liked it for making one decision as simple as it gets, but I can add some code to figure out the difference at build time if |
|
Need some help ? playcanvas.js should be the IIFE. The current examples browser does not consume source, it consumes build products, it also contains an example of making the distinction. |
It's funny that you want to unify UMD and ESM so hard, but then you have such a hard opinion about It's both... So the price of a
That's all wasting time and energy for nothing. When I'm testing a PR or do bug fixing, I do none of that. I use
Thank you for asking, I pretty much just need a node/fs function right now that reliably tells me if a file is UMD or EJS if you want to work it out already. I don't know how much time you have at hand, but I have other things on my TODO list aswell 😅 |
|
I don't want to unify anything. There needs to be multiple build products. UMD is an abandoned pattern that hasn't become a standard. It has drawbacks, incompatibilities with the last 8 years of standards, and you can ship 2 other formats covering all use cases, never having an issue. Mix and match them, etc. The source is at a ES11 language level, trying to consume it directly as ES5 or make any distinctions like that won't go very far. One reason for the build system is to translate that down on at least one build product to the lowest common support target which is ES5-ish. This is why engine build products are consumed by the examples app to create an ES5 build. I understand the confusion and complexity. Things can be simplified; external requirements are forcing some extra machinery. |
|
To answer your question: You'll already know from the build file name what you are importing. Importing either incorrectly won't build. Still.. Off the top of my head, if an extra check at runtime is required for some reason, the node function you are looking for is the humble "import" statement; the same why you would when you want "side effects" from a polyfill. import './file.js';It works on both modules and IIFEs. If the global variable "pc" is defined (the side effect), you imported the IIFE. If not, you have the module. |
@epreston I don't have a sense we speak the same language here, but thank you for trying to help, I will implement my requirements myself. I'm not sure why you bring up And our ES5 |
|
No worries. The code you have linked and reviewed is for selecting build products from the build directory. Should be safe to use the existing code "as is" for selecting between an ES5 or modular build. It has nothing to do with source. Read up on UMD. Having universal in the name hasn't helped it. A UMD is an IIFE with less compatibility. There are a few issues and omissions with the build products. That's one of them. |
Wrong. A I wrote:
You wrote:
Response: You can't admit that you were wrong once. |
|
Dig into it when you have time. |
I don't know whom you are trying to fool here, but anyway: Lines 288 to 291 in 3032623 I'm out of this PR. |
|
A UMD is a pattern of writing an IIFE with a few extra Here it is in it's full form. // myModuleName.js
(function (root, factory) {
if (typeof define === 'function' && define.amd) {
// AMD. Register as an anonymous module.
define(['exports', 'b'], factory);
} else if (typeof exports === 'object' && typeof exports.nodeName !== 'string') {
// CommonJS
factory(exports, require('b'));
} else {
// Browser globals
factory((root.myModuleName = {}), root.b);
}
}(typeof self !== 'undefined' ? self : this, function (exports, b) {
// Use b in some fashion.
// attach properties to the exports object to define
// the exported module properties.
exports.action = function () {};
}));AMD and commonjs are useless in the browser space. The library does not run in node, AMD needs a runtime to work in a browser. These extra You are pointing to a mistake in the build configuration. |
|
I just looked at three and they seemed to have marked their UMD build as deprecated: https://github.com/mrdoob/three.js/blob/dev/utils/build/rollup.config.js#L154 |
|
Yep. It was useful in 2016, today, not so much with a supported standard built into the language and browsers. It's a liability if someone uses the "import" statement with the wrong file. Some people like the global namespace with everything in it, one file, instead of a collection of them. Same as this project does with some files in the scripts folder. A plain iife would allow that. You do the chart and you get the same support matrix. Correction, it adds support for an extra use case. |
|
Soooo.....we mergin'? 😄 |
The source (not the build products) for the project are es6 modules. Inform all tools that this is an es6 project.
At the moment, the package.json does not specify this correctly so tools assume "cjs". Some entry points are marked with "mjs" to correct this, but, there will always be a new edge case where incorrect assumption is uncovered.
In other words, this sets the "source" assumption to be "modules", so "js" is assumed to be "mjs", "cjs" can still be used to mark tooling code that needs to be interpreted as node only "cjs". "mjs" is reserved for node only code that needs to be evaluated as a module.
Renaming all the files to
.mjsis pointless once the project is marked correctly.References: #5623
References: #5555
Fixes the odd amount of effort expended juggling file extensions for js files.
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.