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 web standard modules #7365

Merged
merged 8 commits into from Nov 3, 2023

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Oct 27, 2023

Literally just makes sure we're importing with .js extensions everywhere, so the output has those extensions as well.

@benlesh
Copy link
Member Author

benlesh commented Oct 27, 2023

Okay the sad thing is I had to change how we were running Mocha tests to build the .js files and then run them. Which sort of sucks because it takes 4x longer... but it's still 12s vs 4s.

It's probably worth investigating ditching mocha or looking into a better test runner set up. But that's a huge undertaking and I really don't want to deal with it. I think the trade off here is worth it.

Adds an esm import integration test and updates the commonjs integration test.
@benlesh
Copy link
Member Author

benlesh commented Oct 27, 2023

I also fixed our existing import integration test to work a little better with this setup, and I added another test to make sure that esm imports are working okay.

const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

async function main() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This integration test will load up index.html (in the same directory) and check <script type="module"></script> web module imports.

See browser-test.js for the other half of these tests.

@@ -0,0 +1,35 @@
import { Observable, from, map } from './node_modules/rxjs/dist/esm/index.js';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a rough set of imports and tests to assert that module imports are working okay.


assert(results.length === 3, 'from should work');

window.reportDone(success);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called with true if everything is fine and false if the there was any problem at all.

<title>RxJS Import Integration Test</title>
</head>
<body>
<script type="module" src="./browser-test.js"></script>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest, this is the first "real" <script type="module"> I've ever written.

@43081j
Copy link
Contributor

43081j commented Oct 28, 2023

you also want to add the .js paths to the package exports right? many projects use standard imports (.js) via typescript, which will check against the exports these days (and in this case will fail since the full paths are not exported)

& your build before your test is necessary in master too if you remove the path mappings patch you had. i wouldn't move off mocha because of it

@benlesh
Copy link
Member Author

benlesh commented Oct 29, 2023

you also want to add the .js paths to the package exports right?

Doesn't seem to be necessary.

@43081j
Copy link
Contributor

43081j commented Oct 29, 2023

are you sure?

if i import something like rxjs/dist/esm/ajax/index.js with nodenext resolution, afaik it will check the package exports for that path.

i could import rxjs/ajax but other linting would complain since the browser wouldn't know what that is (e.g. required file extensions, real paths required, etc. aliases are a node thing)

just wanna be sure we get this right 🙏 i could be wrong ofc. we can export wildcards i think btw, to avoid having to write each one manually in the map

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's ugly - the .js in the .ts files - but it works.

@benlesh
Copy link
Member Author

benlesh commented Nov 3, 2023

just wanna be sure we get this right 🙏

Feel free to add a test. Right now at least we know it works with node mjs, cjs, web modules, vite, and webpack.

@benlesh
Copy link
Member Author

benlesh commented Nov 3, 2023

BTW: It looks like we already had .js extensions on the exports?

"types": "./dist/types/index.d.ts",
"node": "./dist/cjs/index.js",
"require": "./dist/cjs/index.js",
"default": "./dist/esm/index.js"
},
"./ajax": {
"types": "./dist/types/ajax/index.d.ts",
"node": "./dist/cjs/ajax/index.js",
"require": "./dist/cjs/ajax/index.js",
"default": "./dist/esm/ajax/index.js"
},
"./fetch": {
"types": "./dist/types/fetch/index.d.ts",
"node": "./dist/cjs/fetch/index.js",
"require": "./dist/cjs/fetch/index.js",
"default": "./dist/esm/fetch/index.js"
},
"./operators": {
"types": "./dist/types/operators/index.d.ts",
"node": "./dist/cjs/operators/index.js",
"require": "./dist/cjs/operators/index.js",
"default": "./dist/esm/operators/index.js"
},
"./testing": {
"types": "./dist/types/testing/index.d.ts",
"node": "./dist/cjs/testing/index.js",
"require": "./dist/cjs/testing/index.js",
"default": "./dist/esm/testing/index.js"
},
"./webSocket": {
"types": "./dist/types/webSocket/index.d.ts",
"node": "./dist/cjs/webSocket/index.js",
"require": "./dist/cjs/webSocket/index.js",
"default": "./dist/esm/webSocket/index.js"
},
"./internal/*": {
"types": "./dist/types/internal/*.d.ts",
"node": "./dist/cjs/internal/*.js",
"require": "./dist/cjs/internal/*.js",
"default": "./dist/esm/internal/*.js"
},

@benlesh benlesh merged commit 0095c26 into ReactiveX:master Nov 3, 2023
3 checks passed
@benlesh benlesh deleted the output-esm-modules branch November 3, 2023 15:28
@43081j
Copy link
Contributor

43081j commented Nov 3, 2023

its that the exported path (./ajax for example in what you linked) isn't .js, and isn't "real".

i.e. in a repo where we're required to have file extensions on imports, we'd have to explicitly ignore rxjs/ajax in whatever tooling is enforcing the extensions.

basically because the real paths are not exported. so it may be we just need ./dist/esm/* as an exported path, and similarly, ./dist/cjs/*.

otherwise we would try import whatever from 'rxjs/dist/esm/ajax/index.js'; and typescript would throw (amongst other tools that enforce package exports).

it only really matters in projects with that level of strictness on imported paths though (e.g. one which isn't bundled, so needs the exact relative path rather than a non-existent alias). but would be nice to sort at some point

@benlesh
Copy link
Member Author

benlesh commented Nov 3, 2023

I guess what I'm saying is: I can't prioritize this right now. If you want to set up an integration test that proves something is broken without such a change, and then make the change, that would be welcome.

@43081j
Copy link
Contributor

43081j commented Nov 3, 2023

Ofc, no worries. Just wanted to explain what I meant

Definitely belongs in another pr. If I get time some day I'll have a stab at it

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.

None yet

3 participants