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

convert to JS #38

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

convert to JS #38

wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Owner

No description provided.

@Conduitry
Copy link
Collaborator

"Buble", wow, that's a blast from the past.

@Rich-Harris
Copy link
Owner Author

yeah, still the easiest way to turn const to var

Copy link

@ansballard ansballard left a comment

Choose a reason for hiding this comment

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

Saw this PR sitting and just figured I'd review it if that's helpful. Don't see any issues, mostly just noting minor functional changes between js and ts, and a couple places where types were lost.


const VERSION = "__VERSION__";

export { transform, define, load, VERSION };

Choose a reason for hiding this comment

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

[comment]
would be nice to see this diff as a git mv, but minimal changes so it looks fine

* @param {(__import: __Import, __exports: __Exports, ...deps: any[]) => void} factory
*/
export function define(id, deps, factory) {
const __import = (dep) => load(new URL(dep, id).toString());

Choose a reason for hiding this comment

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

[comment]
just making a note that toString was added, but it looks like the typescript version expected a string | URL but then just cast to string, so expected functionality should be the same

// for browsers without `URL`
return <unknown>(0,eval)(code);
}
}

Choose a reason for hiding this comment

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

[comment]
same here, diff would be happier as a git mv


function tokenClosesExpression() {
if (lsc() === ')') {
var c = parenMatches[lsci];
let c = parenMatches[lsci];

Choose a reason for hiding this comment

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

[comment]
noting functional change, var -> let, yay scope

if (a.name === 'default') return 1;
if (b.name === 'default') return -1;
})
.forEach((s: Specifier) => {
.forEach((s) => {

Choose a reason for hiding this comment

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

[comment]
lost some types in these sort/forEach methods, doesn't seem like a big deal

@@ -658,7 +766,7 @@ export function transform(source: string, id: string) {

let transformed = `__shimport__.define('${id}', [${deps}], function(${names}){ ${hoisted.join('')}`;

const ranges: any[] = [
const ranges = [

Choose a reason for hiding this comment

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

[comment]
lost a type here, any[] can probably just get inferred since it's initialized as an array but it might still be worth keeping?

@jimmywarting
Copy link

bump

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.

4 participants