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

Detect type="module" #450

Closed
wants to merge 2 commits into from
Closed

Conversation

TimDaub
Copy link

@TimDaub TimDaub commented Jul 14, 2020

Fixes #449

This is merely an attempt to fix the above mentioned issue. The problem with this fix this that so far it only detects if the Javascript modules feature is available in the browser. However, a developer could add a regular <script> tag in a browser that supports modules. Then this statement would still run.

I don't see it much of a problem though, as the added statement runs in addition to the statements below, fixing it for the problems described in the linked issue #449

@kewisch
Copy link
Owner

kewisch commented Jul 15, 2020

This seems like a viable quick fix. What I'd like to aim for is modernizing ical.js to use es6 modules instead of trying to cram everything onto a global variable, but still keep the ICAL prefix on the outside.

Can you add a comment inside the if block what this is for? Also, you may need to adjust the jshint and istanbul comments before so they apply to the correct blocks.

@TimDaub TimDaub force-pushed the feat/detect-module-mode branch from 390a148 to 04236c7 Compare July 15, 2020 08:27
@TimDaub
Copy link
Author

TimDaub commented Jul 15, 2020

What I'd like to aim for is modernizing ical.js to use es6 modules

👍 Godspeed! Looking forward to those changes.

Can you add a comment inside the if block what this is for?

Left a comment.

Also, you may need to adjust the jshint and istanbul comments before so they apply to the correct blocks.

Adjusted the comments.

@TimDaub
Copy link
Author

TimDaub commented Jul 20, 2020

bump.

@TimDaub
Copy link
Author

TimDaub commented Jul 23, 2020

Update: I noticed that when executing ical.js in nodejs, HTMLScriptElement isn't available from the global space and so
the ical.js wouldn't be executable.

So in fb1e238, I'm now making sure that window is available and only then I'm performing the module check.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 96.958% when pulling fb1e238 on TimDaub:feat/detect-module-mode into dcfe5a6 on mozilla-comm:master.

@TimDaub
Copy link
Author

TimDaub commented Jan 6, 2021

👋🏼 Hey,

this PR has now been open for half a year. I prefer to use the actual ical.js in my application and not continue to rely on my personal fork. So can you please come to a decision about merging this PR? Thanks!

Best,
Tim

@kewisch
Copy link
Owner

kewisch commented Jul 13, 2021

I appreciate the work you've done here, even if the timeline for responding may not suggest so. I'm going to add your case to #470 and get it fixed there, let me know if you can think of a case where window is not defined but HTMLScriptElement is.

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.

cannot be used in type="module" mode
3 participants